-
Notifications
You must be signed in to change notification settings - Fork 120
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
Use parent pom 4.77 #219
Use parent pom 4.77 #219
Conversation
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.
Please remove the pom.xml.versionsBackup
file. It is not needed and will distract others.
ok |
Please retain the pull request template, including the The pull request template means it when it says: Testing doneProvide a clear description of how this change was tested. |
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.
@shivajee98 I told you in the earlier review of the pull request to split the pull request into multiple pull requests. You've again combined multiple changes into a single pull request. Please don't do that. It makes the change more difficult to review and makes the maintainer less likely to accept the change.
While code was going through the checks then SpotBugs identified issue of null exception, so i made the change here |
That makes sense. Thanks for the clarification. |
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.
Please remove the IDE settings file .vscode/settings.json
from the pull request. That setting is specific to your IDE and is unrelated to the topic of the pull request.
Please add a "Testing done" section to the pull request description and describe the testing you used to confirm this change is correct and complete. |
Ok sure |
I made some style changes in 02e1fb3 because I prefer to define the variable near its use so that it is clear that the variable I made some spacing changes in 02e1fb3 because I like to keep the number of changes small between versions. I did those myself rather than bothering you with them because they are purely questions of style and this plugin does not have a contributing guide that offers an opinion on those questions of style. Are those changes acceptable to you @shivajee98 ? If so, then once the CI job passes, I believe this is ready to be squash merged. |
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.
Thanks.
Since the previous behavior with a null projectName would have been to throw a null pointer exception, I think it is OK to throw an illegal argument exception.
mvn versions:update-parent
Added null check in CopyArtifact.java and handle IllegalArgumentException
Testing Details:
mvn clean install
on CodesapceSubmitter checklist