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

Fix Eclipse compiler warnings #1042

Merged
merged 15 commits into from Dec 15, 2014
Merged

Fix Eclipse compiler warnings #1042

merged 15 commits into from Dec 15, 2014

Conversation

marcphilipp
Copy link
Member

I've split the various fixes into separate commits to illustrate why/how I fixed certain warnings.

@marcphilipp
Copy link
Member Author

This fixes all compiler warnings in Eclipse.

@marcphilipp marcphilipp added this to the 4.13 milestone Dec 8, 2014
@@ -73,6 +73,7 @@
* @see TestResult
* @see TestSuite
*/
@SuppressWarnings("deprecation")
Copy link
Member

Choose a reason for hiding this comment

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

I actually prefer that we suppress warnings at the narrowest place possible. There's a lot of code in this class, and I'd like to see if we introduce new references to deprecated code

Copy link
Member Author

Choose a reason for hiding this comment

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

Generally I totally agree with you. In this case, however, adding @SuppressWarnings on the class level is the only way to get rid of all deprecation warnings in TestClass because its superclass Assert is deprecated on the class-level.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should have just deprecated the methods in Assert instead of the whole class, but I think that ship has sailed :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

So, okay by you?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this is fine

@marcphilipp
Copy link
Member Author

@kcooney I've added another commit to that fixes the serialization warning by adding a serivalver generated serialVersionUID (see #1042 (comment)).

@kcooney
Copy link
Member

kcooney commented Dec 14, 2014

LGTM

@marcphilipp
Copy link
Member Author

Good enough to merge it?

@kcooney
Copy link
Member

kcooney commented Dec 15, 2014

Sorry, LGTM = "looks good to me" => "feel free to submit" :-)

I was going to let you merge this because I wasn't sure if you wanted to have multiple commits or squash it (either is fine)

@marcphilipp
Copy link
Member Author

Ok, thanks! 😄

I think I'll remove the ValidationError change from a170a3d and squash it with 08e6b86. For the rest, I think it's better to have separate commits in this case.

marcphilipp added a commit that referenced this pull request Dec 15, 2014
@marcphilipp marcphilipp merged commit bc0292b into master Dec 15, 2014
@marcphilipp marcphilipp deleted the fix-compiler-warnings branch December 15, 2014 19:45
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

2 participants