Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove deprecated method #8810

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MGaetan89
Copy link
Contributor

@MGaetan89 MGaetan89 commented Feb 8, 2024

These methods were supposed to be removed in older versions of Robolectric:

Symbol Target removal version Open Source usages
ConfigMerger 4.3 usages
RobolectricTestRunner.buildGlobalConfig() 4.3 usages
ServiceController.withIntent(Intent) 3.6 usages

Note: I've marked RobolectricTestRunner.CONFIG_PROPERTIES as deprecated, because it is no longer used in the project, but it's still public. Let me know if you want to proceed otherwise.

@MGaetan89 MGaetan89 force-pushed the feature/remove_deprecated_code branch from 499e022 to 9bc74b6 Compare February 8, 2024 22:00
Copy link
Member

@utzcoz utzcoz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MGaetan89 IIRC, many deprecated methods can not be deleted directly because widely used. Sorry about it.

@MGaetan89
Copy link
Contributor Author

No worry. I saw that there are many deprecated methods in the projects, but only removed these because there is a removal version in te deprecation message.
If you prefer to keep them, feel free to close this PR.

@hoisie
Copy link
Contributor

hoisie commented Feb 13, 2024

Thanks for doing this, I will take a closer look at this tomorrow.

@MGaetan89 MGaetan89 force-pushed the feature/remove_deprecated_code branch from 9bc74b6 to fa1a0da Compare February 13, 2024 13:16
@hoisie
Copy link
Contributor

hoisie commented Feb 14, 2024

ServiceController.withIntent(Intent)

Looks like there are some tests at Google that are using ServiceController.withIntent(Intent) to deliver new intents to an existing Service object. This appears to be a valid testing use case and there does not appear to be an alternative API for this. This has also come up here as well: #3277 (comment).

Anyway, long-story short, we probably need to keep ServiceController.withIntent(Intent). Maybe we should just un-deprecate it. Thoughts @brettchabot and @utzcoz?

@hoisie
Copy link
Contributor

hoisie commented Feb 14, 2024

RobolectricTestRunner.buildGlobalConfig()

Looks like this is used by Litho:

https://github.com/facebook/litho/blob/master/litho-testing/src/main/java/com/facebook/litho/testing/testrunner/LithoTestRunner.java#L80-L82

We should check if LithoTestRunner can be updated to remove that.

Other than these two, I am not seeing any compatibility issues.

@utzcoz
Copy link
Member

utzcoz commented Feb 14, 2024

@hoisie I recommend to clean-up remove time in comments instead of removing them. It's very hard to find all potential usages now. If we really want to remove them, we can remove them in multiple Robolectric versions but from deprecated first.

Maybe two years ago, I still want to clean-up these methods, but I found there were some important methods that marked by deprecated were used by many projects we can't provide alternative stable methods for them. I know it's great if we can let developers use standard APIs of Robolectric, instead of some helper methods like configuration APIs for purpose usage.

@utzcoz
Copy link
Member

utzcoz commented Feb 15, 2024

After reviewing code again, if we can confirm that we can use standard APIs to retrieve Display's metrics and real metrics, we can remove these two deprecated APIs first instead of full list that the current PR wants to remove.

@MGaetan89
Copy link
Contributor Author

After reviewing code again, if we can confirm that we can use standard APIs to retrieve Display's metrics and real metrics, we can remove these two deprecated APIs first instead of full list that the current PR wants to remove.

I've tried to do just that (removing ShadowDisplay.getMetrics(DisplayMetrics) and ShadowDispaly.getRealMetrics(DisplayMetrics)), and keep everything else. But since these methods also alter the scaledDensity, I'd say that we need to remove ShadowDisplay.setScaledDensity(float) as well.
If I remove these three methods, the only change needed to make the tests pass is to remove the part changing the scaled density.

If that's fine by you, I can update this PR as explained. Otherwise, if the other methods need to be kept, I'd say that we can close this PR.

@hoisie
Copy link
Contributor

hoisie commented Feb 15, 2024

I am OK with having a PR that removes all of the methods on ShadowDisplay

@hoisie
Copy link
Contributor

hoisie commented Feb 15, 2024

If we really want to remove them, we can remove them in multiple Robolectric versions but from deprecated first.

These methods have been deprecated for years right?

In general I am OK with deleting things even if teams have to fix small things when they upgrade.

Robolectric has a MASSIVE public API surface and it's still growing. I think it's OK to be a little aggressive about removing stuff that has been deprecated.

People can always use previous versions for as long as they need.

@utzcoz
Copy link
Member

utzcoz commented Feb 15, 2024

@MGaetan89 If you would like, you can send a new PR to delete deprecated methods for ShadowDisplay only.

@MGaetan89
Copy link
Contributor Author

MGaetan89 commented Feb 15, 2024

Here's the PR to remove every deprecated methods in ShadowDisplay: #8840

These methods have been deprecated for years right?

All the methods were meant to be removed in Robolectric 3.7, which was released in 02.2018. So they've been deprecated for a few years now, yes.

@utzcoz
Copy link
Member

utzcoz commented Feb 16, 2024

In general I am OK with deleting things even if teams have to fix small things when they upgrade.

I agree with you that developers only need to modify small patch for a new version. But if deleted APIs are important configuration related APIs, it is always very easily. I have seen some people customize the Robolectric a lot based on these deprecated internal APIs.

The adaption might be easier if developers can replace it with other public APIs, but some APIs without replacements. If we delete the deprecated APIs, developers who use them need to delete related tests as they can't use other public APIs to make related tests work again.

@MGaetan89 MGaetan89 force-pushed the feature/remove_deprecated_code branch from fa1a0da to 513aff7 Compare February 16, 2024 09:08
@MGaetan89
Copy link
Contributor Author

I've updated the description of the PR to includes links to usages of each deprecated method.
I've used GitHub's search and excluded HTML and Markdown files, to ignore documentation usages.

These methods were supposed to be removed in older versions of Robolectric
@MGaetan89 MGaetan89 force-pushed the feature/remove_deprecated_code branch from 513aff7 to 71832ba Compare March 14, 2024 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants