-
Notifications
You must be signed in to change notification settings - Fork 127
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 #282 Better document include* and exclude* parameters in AbstractAddThirdPartyMojo #273
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.
Tried to improve the wording and sometimes the meaning (not sure I way correct in all cases).
src/main/java/org/codehaus/mojo/license/AbstractAddThirdPartyMojo.java
Outdated
Show resolved
Hide resolved
src/main/java/org/codehaus/mojo/license/AbstractAddThirdPartyMojo.java
Outdated
Show resolved
Hide resolved
src/main/java/org/codehaus/mojo/license/AbstractAddThirdPartyMojo.java
Outdated
Show resolved
Hide resolved
src/main/java/org/codehaus/mojo/license/AbstractAddThirdPartyMojo.java
Outdated
Show resolved
Hide resolved
src/main/java/org/codehaus/mojo/license/AbstractAddThirdPartyMojo.java
Outdated
Show resolved
Hide resolved
src/main/java/org/codehaus/mojo/license/AbstractAddThirdPartyMojo.java
Outdated
Show resolved
Hide resolved
src/main/java/org/codehaus/mojo/license/AbstractAddThirdPartyMojo.java
Outdated
Show resolved
Hide resolved
src/main/java/org/codehaus/mojo/license/AbstractAddThirdPartyMojo.java
Outdated
Show resolved
Hide resolved
src/main/java/org/codehaus/mojo/license/AbstractAddThirdPartyMojo.java
Outdated
Show resolved
Hide resolved
src/main/java/org/codehaus/mojo/license/AbstractAddThirdPartyMojo.java
Outdated
Show resolved
Hide resolved
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 still see some unresolved comments from yesterday. Could you please address them?
src/main/java/org/codehaus/mojo/license/AbstractAddThirdPartyMojo.java
Outdated
Show resolved
Hide resolved
* This filter is applied on artifacts gathered as direct dependencies (and | ||
* their transitive) of the projects in the reactor. | ||
* <p> | ||
* By default if an artifact is being excluded its transitive dependencies |
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.
Remove "being"
* or {@code -Dlicense.includedScopes=test,provided}. | ||
* <p> | ||
* This filter is applied on artifacts gathered as direct dependencies (and | ||
* their transitive) of the projects in the reactor. |
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.
Add "dependencies" after "transitive"
done |
I still see unresolved comments from yesterday, e.g. here 404d41a#r258679848 and here 404d41a#r258680088 |
For Remove "being" and "Add "dependencies" after "transitive"" are just single comment without review so I can not mark them as "Resolve conversation" |
I do not care for the status of my comments. The issues in code need to get addressed. I can still see 3 occurrences of "is being" in the file. Similar for "transitive)" and perhaps others. Please go through the comments and make sure they are either fixed or insist on your text if you think I am wrong. |
I have removed any "is being" and "transitive" +" dependencies" in the javadoc |
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.
@nfalco79 I have sent a PR against your branch https://github.com/nfalco79/license-maven-plugin/pull/1 that you may want to review and eventually amend to your commit.
Please reword your commit message to Fix #282 Better document include* and exclude* parameters in AbstractAddThirdPartyMojo
good, seems the fastest solution. Thank you |
Let me reword commit |
I'd prefer to have only one commit here. Could you please squash the two into one and change the commit message to |
…AddThirdPartyMojo
Squashed |
This PR to explain better include exclude properties for #218