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

A PR for issue#405. #410

Merged
merged 1 commit into from
Mar 2, 2022
Merged

A PR for issue#405. #410

merged 1 commit into from
Mar 2, 2022

Conversation

SteafanMrZhou
Copy link
Contributor

I'm so sorry to delay raising a pull request for issue#405,cause i'm very busy a period ago.
I annotated some method overloads by @OverRide,it is necesseary for java project.
There are others like this:curly braces should follow the class name immediately, not on another line,etc.
Awaiting your review,thanks.

@SteafanMrZhou
Copy link
Contributor Author

#405

@Palmr Palmr requested review from swarren12 and Palmr January 5, 2022 09:58
Copy link
Contributor

@Palmr Palmr left a comment

Choose a reason for hiding this comment

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

These all seem fine to me

@Palmr
Copy link
Contributor

Palmr commented Jan 5, 2022

All the places you have added the @Override annotation seem appropriate and I'm fairly surprised they were missing in the first place. Good spot 👍


A couple of points on your comments though:

it is necesseary for java project

The @Override annotation is good practice as it makes it easy to spot methods that are overriding and the compiler can throw an error if you annotate a method with it that is not actually overriding a method on a parent class/interface.

But it is not really necessary, if you do not have it, the bytecode is no different and the running of your application is no different. So @Override annotations being being missing is not a bug and adding them is not an optimisation.

But I would agree it's better to have them than not have them.

There are others like this:curly braces should follow the class name immediately, not on another line,etc.

This change would be a style change only, and would go against the checkstyle rules currently in this project.

Do not spend time on this as there is no benefit and the PR will be rejected.

@Palmr Palmr merged commit a32218d into LMAX-Exchange:master Mar 2, 2022
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