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

refactor: remove @Deprecated methods #3184

Merged
merged 7 commits into from
Dec 9, 2019
Merged

Conversation

monperrus
Copy link
Collaborator

@monperrus monperrus commented Dec 1, 2019

Remove @deprecated methods and types.

Cool factor: I used Spoon in sniper mode for doing this, see method removeDeprecatedMethods. It worked, kudos to the sniper mode!

.cvsignore is removed because it crashes the sniper pretty-printer.

@monperrus
Copy link
Collaborator Author

Not so many resources on automated removal of deprecated methods, a serious one is Drupal 9: Automated Deprecated Code Removal - A Proof of Concept

@monperrus
Copy link
Collaborator Author

ready to merge

@@ -231,94 +228,6 @@ private boolean isTypePresentInStaticImports(String type, Collection<String> sta
return imports;
}

@Test
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess the whole class should be renamed since ImportScanner does not exist anymore

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually I'm not really comfortable with removing those tests: shouldn't they be refactored?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you're right the rest if ImportScannerTest has been moved to ImoprtTest

}

@Test
public void testremoveDeprecatedMethods() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be better to put an ignore on the test, instead of commenting its content, wdyt?

@monperrus
Copy link
Collaborator Author

monperrus commented Dec 5, 2019 via email

@monperrus
Copy link
Collaborator Author

pushed changes

@monperrus
Copy link
Collaborator Author

fixed ci

@surli
Copy link
Collaborator

surli commented Dec 9, 2019

I'm not in favor of Ignored tests, this usually results in maintenance and technical debt.

Well, I'm not a huge fan of commented code, which I find harder to find for maintenance: at least with ignores you're sure that the test is still compiling and you know it's there with the test result.
Now it definitely isn't a blocker for this PR.

Re the refactoring of tests, I didn't check those in the details so I trust you on this one, but I guess we should add a coverage threshold at a point :)

@surli surli merged commit 8ff4eb4 into INRIA:master Dec 9, 2019
@monperrus
Copy link
Collaborator Author

monperrus commented Dec 9, 2019 via email

@monperrus monperrus mentioned this pull request Mar 20, 2020
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.

2 participants