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

refactoring: remove 'final' modifier from all arguments of method/c-tor at ImportControlCheck and around #4353

Closed
romani opened this Issue May 13, 2017 · 15 comments

Comments

Projects
None yet
5 participants
@romani
Member

romani commented May 13, 2017

problem was found at #4282 (comment)

New code added a lot of unnecessary final in parameters of methods and constructors.
We have a guard Check that catch reusage of argument, so this finals in these areas just make code verbose.
We should remove all final in parameters for these areas.

Fix should be done after PR #4282 is merged.

@sharang108

This comment has been minimized.

Show comment
Hide comment
@sharang108

sharang108 May 16, 2017

Contributor

I want to contribute, but I don't know where to begin after pulling the source code.

Contributor

sharang108 commented May 16, 2017

I want to contribute, but I don't know where to begin after pulling the source code.

@sharang108

This comment has been minimized.

Show comment
Hide comment
@sharang108

sharang108 May 17, 2017

Contributor

How can I contribute without using IDEs?

Contributor

sharang108 commented May 17, 2017

How can I contribute without using IDEs?

@jochenvdv

This comment has been minimized.

Show comment
Hide comment
@jochenvdv

jochenvdv May 17, 2017

Contributor

These steps can be applied to any text editor or IDE (see "Beginning Development"):

  1. Fork the repository
  2. Create and switch to a new branch for your fixes
  3. Implement your changes using an editor of choice
  4. Run tests and code analysis using mvn clean verify from the command line
  5. Commit and push your fixes to your fork
  6. View your forked repo on GitHub and create a PR against the original Checkstyle master branch, make sure to adhere to the PR rules mentioned above
Contributor

jochenvdv commented May 17, 2017

These steps can be applied to any text editor or IDE (see "Beginning Development"):

  1. Fork the repository
  2. Create and switch to a new branch for your fixes
  3. Implement your changes using an editor of choice
  4. Run tests and code analysis using mvn clean verify from the command line
  5. Commit and push your fixes to your fork
  6. View your forked repo on GitHub and create a PR against the original Checkstyle master branch, make sure to adhere to the PR rules mentioned above
@sharang108

This comment has been minimized.

Show comment
Hide comment
@sharang108

sharang108 May 19, 2017

Contributor

I am ready, what to do next?

Contributor

sharang108 commented May 19, 2017

I am ready, what to do next?

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach May 19, 2017

Member

@sharang108 See jochenvdv's post. It lists all the steps you need to do.
When you are done, there will be a PR with your changes here that we will review.

Member

rnveach commented May 19, 2017

@sharang108 See jochenvdv's post. It lists all the steps you need to do.
When you are done, there will be a PR with your changes here that we will review.

@sharang108

This comment has been minimized.

Show comment
Hide comment
@sharang108

sharang108 May 20, 2017

Contributor

@rnveach What fixes?? I don't know what to fix, that's why I need something to start with.

Contributor

sharang108 commented May 20, 2017

@rnveach What fixes?? I don't know what to fix, that's why I need something to start with.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach May 20, 2017

Member

I don't know what to fix

@sharang108 Read issue title and first post. I made it more clear what we want done and why. Each issue is something we want changed.
Find all final parameters in methods/constructors/catch in 'main' code and remove them.

Member

rnveach commented May 20, 2017

I don't know what to fix

@sharang108 Read issue title and first post. I made it more clear what we want done and why. Each issue is something we want changed.
Find all final parameters in methods/constructors/catch in 'main' code and remove them.

sharang108 added a commit to sharang108/checkstyle that referenced this issue Jun 20, 2017

Issue #4353: refactoring: remove 'final' modifier from all arguments …
…of method/c-tor at ImportOrderCheck and around
@sharang108

This comment has been minimized.

Show comment
Hide comment
@sharang108

sharang108 Jun 20, 2017

Contributor

@romani @rnveach @MEZk Is this what you are asking for?

Contributor

sharang108 commented Jun 20, 2017

@romani @rnveach @MEZk Is this what you are asking for?

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jun 20, 2017

Member

@sharang108 Yes, in test and in main check.

Member

rnveach commented Jun 20, 2017

@sharang108 Yes, in test and in main check.

@romani romani changed the title from refactoring: remove 'final' modifier from all arguments of method/c-tor at ImportOrderCheck and around to refactoring: remove 'final' modifier from all arguments of method/c-tor at ImportControlCheck and around Jun 21, 2017

sharang108 added a commit to sharang108/checkstyle that referenced this issue Jun 21, 2017

Issue #4353: refactoring: remove 'final' modifier from all arguments …
…of method/c-tor at ImportOrderCheck and around

sharang108 added a commit to sharang108/checkstyle that referenced this issue Jun 21, 2017

Issue #4353 refactoring: remove 'final' modifier from all arguments o…
…f method/c-tor at ImportControlCheck and around

sharang108 added a commit to sharang108/checkstyle that referenced this issue Jun 21, 2017

Issue #4353: refactoring: remove 'final' modifier from all arguments …
…of method/c-tor at ImportControlCheck and around

sharang108 added a commit to sharang108/checkstyle that referenced this issue Jun 22, 2017

Issue #4353: refactoring: remove 'final' modifier from all arguments …
…of method/c-tor at ImportControlCheck and around

sharang108 added a commit to sharang108/checkstyle that referenced this issue Jun 22, 2017

Issue #4353: refactoring: remove 'final' modifier from all arguments …
…of method/c-tor at ImportControlCheck and around

sharang108 added a commit to sharang108/checkstyle that referenced this issue Jun 22, 2017

Issue #4353: refactoring: remove 'final' modifier from all arguments …
…of method/c-tor at ImportControlCheck and around

sharang108 added a commit to sharang108/checkstyle that referenced this issue Jun 22, 2017

Issue #4353: refactoring: remove 'final' modifier from all arguments …
…of method/c-tor at ImportControlCheck and around

romani added a commit that referenced this issue Jun 23, 2017

Issue #4353: refactoring: remove 'final' modifier from all arguments …
…of method/c-tor at ImportControlCheck and around
@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 23, 2017

Member

@sharang108 , did you finish the issue ?

Member

romani commented Jun 23, 2017

@sharang108 , did you finish the issue ?

@sharang108

This comment has been minimized.

Show comment
Hide comment
@sharang108

sharang108 Jun 23, 2017

Contributor

@romani Yes, but just confirm with @rnveach

Contributor

sharang108 commented Jun 23, 2017

@romani Yes, but just confirm with @rnveach

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 23, 2017

Member

fix is merged.

Member

romani commented Jun 23, 2017

fix is merged.

@romani romani closed this Jun 23, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 23, 2017

Member

@rnveach , it might be good to have such a Check in checkstyle to forbid final on arguments, what do you think ?

Member

romani commented Jun 23, 2017

@rnveach , it might be good to have such a Check in checkstyle to forbid final on arguments, what do you think ?

@rnveach

This comment has been minimized.

Show comment
Hide comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment