-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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 some typos on code and javadoc on maven devtool project #36320
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.
Most of the changes are for the better, thanks! But I have a question on one of them.
.filter(n -> Arrays.stream(Deployer.values()).anyMatch(e -> e.equals(n))) | ||
.filter(n -> Arrays.stream(Deployer.values()).anyMatch(e -> false)) |
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.
Could you explain this particular change?
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 so much @gsmet for the review, I can say that Result of 'e.equals(n)
' is always 'false
' for the reason that ‘equals
' between objects of inconvertible types 'Deployer
' and 'String
’, So I made this shortcut
I think this class deserves to write a test for it’s own
Please let me know if I missed something with that, I can rollback this change the time to have some tests for Deployer enum
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.
Hum. This is a bug, and not a pretty one. I recommend you keep it the way it is for now (i.e. get back to the previous buggy behavior) and we will address it in a follow-up PR as we need to discuss it.
@iocanel I think you wanted to either compare with e.name()
or e.extension
. I'm not completely sure about the fix as I'm a bit unsure of how you handle knative. I don't see an extension for it so I suppose you would have kubernetes instead and that knative can't really be identified this way?
If so, it might be better to drop .map(d -> strip(d.getArtifactId()))
entirely and compare on the extension rather than the name.
I will let you have a look.
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.
@gsmet, @iocanel I made a change on the last commit for this pull request on the related instruction, please let me know if you think it can be ok as a first try, I am ready to change it again to fit the desired solution, and on that topic I am in need of your help
.filter(n -> Arrays.stream(Deployer.values()).anyMatch(e -> e.extension.equals(n)))
Thanks
@@ -79,7 +79,7 @@ public static Set<String> getProjectDeployer(MavenProject project) { | |||
return project.getDependencies().stream() | |||
.filter(d -> QUARKUS_GROUP_ID.equals(d.getGroupId())) | |||
.map(d -> strip(d.getArtifactId())) | |||
.filter(n -> Arrays.stream(Deployer.values()).anyMatch(e -> false)) | |||
.filter(n -> Arrays.stream(Deployer.values()).anyMatch(e -> e.extension.equals(n))) |
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 is more complex than that. At the minimum, the .map(d -> strip(d.getArtifactId()))
needs to be removed.
I created #36325 for @iocanel to have a closer look as I'm not entirely sure what's the right fix.
I would recommend to just revert the changes related to that so that we can get this merged as it might take some time for Ioannis to have a closer look at this.
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.
@gsmet I made revert, to keep the existing behaviour on the last commit
3c73d37
to
e56277f
Compare
7b0f6ce
to
6a2c652
Compare
Thanks! I adjusted one small thing and squashed the commits. Let's wait for CI and merge. |
This comment has been minimized.
This comment has been minimized.
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
6a2c652
to
791b933
Compare
Status for workflow
|
This pull request is to fix some typos on java doc and java method name and some minor cleanup