Skip to content
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

Return root node even though everything is filtered. #6846

Merged
merged 1 commit into from
Dec 20, 2023

Conversation

sdedic
Copy link
Member

@sdedic sdedic commented Dec 14, 2023

Two fixes:

  • first, the Scope was designed to be equals based on its name only; the implementation Class was checked by mistake.
  • maven implementation happily filters out even the root of the project when scope filter is active. This is bad, because all data under the root are lost. This PR handles the root conversion specially - root corresponds to the project itself and needs to be present always, so the filter is disabled for it.

@sdedic sdedic added Maven [ci] enable "build tools" tests VSCode Extension [ci] enable VSCode Extension tests labels Dec 14, 2023
@sdedic sdedic added this to the NB21 milestone Dec 14, 2023
@sdedic sdedic requested review from thurka and MartinBalin December 14, 2023 19:52
@sdedic sdedic self-assigned this Dec 14, 2023
@@ -86,9 +86,6 @@ public final boolean equals(Object obj) {
if (obj == null) {
return false;
}
if (getClass() != obj.getClass()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sorry, but this is wrong. I accept that two scopes with different implementation classes but the same name should be equal, but then please do something like:

if (!(obj instanceof Scope)) {
    return false;
}

I believe generally equals is expected to not crash on CCEs, even if an object of an unrelated type is passed in.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh oh ... mea culpa. Of course.

@sdedic sdedic force-pushed the sdedic/dependency-filter-fix branch from 89c0b50 to f978710 Compare December 17, 2023 11:59
@sdedic
Copy link
Member Author

sdedic commented Dec 17, 2023

Merged fix into the main commit before merge.

Copy link
Contributor

@jhorvath jhorvath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@sdedic sdedic merged commit 795e3a7 into apache:master Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maven [ci] enable "build tools" tests VSCode Extension [ci] enable VSCode Extension tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants