-
Notifications
You must be signed in to change notification settings - Fork 6
added header recipe #35
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
Conversation
|
Please put reference to issue to commit message . |
2f41b01 to
7d777e3
Compare
|
@rdiachenko kindly review. |
rdiachenko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Anmol202005 its a lot of issues here becuase you mixed different logic. Please split this PR into the following:
- PR for header recipe implementation only, no integration with config and parent recipe
- PR for loading checkstyle config given config file uri
- PR for using header recipe with config
Please prioritise smaller PRs with independet changes rather than putting everything into one PR to minimise review time and iteration
|
@rdiachenko |
|
@Anmol202005 the check has |
6368840 to
e090327
Compare
|
@rdiachenko kindly review :) |
| import com.puppycrawl.tools.checkstyle.api.CheckstyleException; | ||
| import com.puppycrawl.tools.checkstyle.api.Configuration; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should replace these Checkstyle specific objects with our own like we did with CheckstyleViolation. This will help to avoid Checkstyle dependencies to be spread all around the recipes and have control over needed fields. To not overcomplicate this PR, I'm ok to fix it as part of the PR, which introduces configuration. @Anmol202005 please make a note in the issue to address this point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done. updated issue: #36
|
@rdiachenko done. |
|
@rdiachenko done. |
|
@rdiachenko Done. |
timurt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM @Anmol202005 thanks for your work!
rdiachenko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is not ideal but already took a lot of time and effort.
Ok to merge after this comment got responed #35 (comment)
Fixes: #25
Added header recipe: Adds headers to Java source files when missing.