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

Simplified usage of class AssociableToAST #3063

Merged
merged 8 commits into from
Dec 11, 2022

Conversation

4everTheOne
Copy link
Contributor

@4everTheOne 4everTheOne commented Feb 4, 2021

According to the documentation in AssociableToAST#toAst() all the Resolved declaration most be asociable to a AST.

Currently this doesn't happen, making it hard do access a node from a ResolvedDeclaration. To work around this we need to make some check that requires a lot of boilerplate like in the example below.

private boolean isAncestorOf(ResolvedTypeDeclaration descendant) {
if (descendant instanceof AssociableToAST) {
Optional<Node> astOpt = ((AssociableToAST<Node>) descendant).toAst();
if (astOpt.isPresent()) {
return wrappedNode.isAncestorOf(astOpt.get());
} else {
return false;
}
} else if (descendant instanceof JavaParserEnumDeclaration) {
return wrappedNode.isAncestorOf(((JavaParserEnumDeclaration) descendant).getWrappedNode());
} else {
throw new UnsupportedOperationException();
}
}

This PR suggest the remove of the type parameter of the class AssociableToAST and use a helper method to convert to the desired type.
To make it more clear lets take a look at the example mentioned above but this time in the new format:

private boolean isAncestorOf(ResolvedTypeDeclaration descendant) {
    return descendant.toAst()
            .filter(node -> wrappedNode.isAncestorOf(node))
            .isPresent();
}

Other example, previously we would have to do something like this:

ResolvedDeclaration resolvedDeclaration = ...;
if (resolvedDeclaration instanceof ResolvedFieldDeclaration) {
    ResolvedFieldDeclaration resolvedField = (ResolvedFieldDeclaration) resolvedDeclaration;
    Optional<FieldDeclaration> fieldDeclaration = resolvedField.toAst();
}

Now we can do it by simple doing:

ResolvedDeclaration resolvedDeclaration = ...;
Optional<FieldDeclaration> fieldDeclaration = resolvedDeclaration.toNode(FieldDeclaration.class);

This also allows add room for more complex examples that were not possible with the previous format, like looking for specific declarations. We can do it now by simply:

Optional<VariableDeclarator> variableDeclarator = resolvedDeclaration.toNode(VariableDeclarator.class);

@codecov
Copy link

codecov bot commented Feb 4, 2021

Codecov Report

Merging #3063 (5eaa434) into master (24925df) will decrease coverage by 0.027%.
The diff coverage is 42.307%.

❗ Current head 5eaa434 differs from pull request most recent head bbcf0cd. Consider uploading reports for the commit bbcf0cd to get more accurate results

Impacted file tree graph

@@               Coverage Diff               @@
##              master     #3063       +/-   ##
===============================================
- Coverage     57.443%   57.415%   -0.028%     
- Complexity      2541      2555       +14     
===============================================
  Files            636       637        +1     
  Lines          33809     33853       +44     
  Branches        5841      5839        -2     
===============================================
+ Hits           19421     19437       +16     
- Misses         12307     12336       +29     
+ Partials        2081      2080        -1     
Flag Coverage Δ
AlsoSlowTests 57.415% <42.307%> (-0.028%) ⬇️
javaparser-core 51.944% <0.000%> (-0.009%) ⬇️
javaparser-symbol-solver 36.646% <42.307%> (-0.001%) ⬇️
jdk-10 57.411% <42.307%> (-0.019%) ⬇️
jdk-11 57.411% <42.307%> (-0.028%) ⬇️
jdk-12 57.411% <42.307%> (-0.028%) ⬇️
jdk-13 57.411% <42.307%> (-0.028%) ⬇️
jdk-14 57.411% <42.307%> (-0.028%) ⬇️
jdk-15 57.405% <42.307%> (-0.031%) ⬇️
jdk-16 57.411% <42.307%> (-0.028%) ⬇️
jdk-8 57.413% <42.307%> (-0.028%) ⬇️
jdk-9 57.405% <42.307%> (-0.028%) ⬇️
macos-latest 57.404% <42.307%> (-0.024%) ⬇️
ubuntu-latest 57.401% <42.307%> (-0.028%) ⬇️
windows-latest 57.395% <42.307%> (-0.028%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...arser/resolution/declarations/AssociableToAST.java 0.000% <ø> (ø)
...r/resolution/declarations/ResolvedDeclaration.java 100.000% <ø> (ø)
...declarations/ResolvedTypeParameterDeclaration.java 25.000% <0.000%> (-0.397%) ⬇️
...thub/javaparser/symbolsolver/JavaSymbolSolver.java 58.088% <0.000%> (-0.431%) ⬇️
...el/declarations/DefaultConstructorDeclaration.java 46.153% <0.000%> (-3.847%) ⬇️
.../declarations/JavaParserAnnotationDeclaration.java 73.529% <0.000%> (-2.229%) ⬇️
...rations/JavaParserAnnotationMemberDeclaration.java 80.000% <0.000%> (-8.889%) ⬇️
...larations/JavaParserAnonymousClassDeclaration.java 75.824% <0.000%> (-0.843%) ⬇️
...declarations/JavaParserConstructorDeclaration.java 50.000% <0.000%> (-2.381%) ⬇️
...eclarations/JavaParserEnumConstantDeclaration.java 87.500% <0.000%> (-12.500%) ⬇️
... and 40 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24925df...bbcf0cd. Read the comment docs.

@4everTheOne
Copy link
Contributor Author

After the conversation with jlerbsc at #1940 (comment) that demonstrated the impact of this changes, decided to update this PR to reduce this impact. To reduce this impact a new AssociableToAst was created with a new name AssociatedWithAST and behavior exactly as described in the first comment.

Also AssociableToAst has been marked as Deprecated for not complying with the expected documentation.

@jlerbsc
Copy link
Collaborator

jlerbsc commented Dec 11, 2022

It would be possible to have a clearer PR by deleting all unnecessary commits.

@jlerbsc
Copy link
Collaborator

jlerbsc commented Dec 11, 2022

After the conversation with jlerbsc at #1940 (comment) that demonstrated the impact of this changes, decided to update this PR to reduce this impact. To reduce this impact a new AssociableToAst was created with a new name AssociatedWithAST and behavior exactly as described in the first comment.

Also AssociableToAst has been marked as Deprecated for not complying with the expected documentation.

Given that the next version of JP will introduce a break in the API (this will be a major version certainly 4.0.0.), I send back my opinion, so that you can integrate this change directly into the existing interface.

@jlerbsc
Copy link
Collaborator

jlerbsc commented Dec 11, 2022

Did you complete your PR?

@4everTheOne
Copy link
Contributor Author

Yes I did. Sorry I forgot to leave a comment.

@jlerbsc jlerbsc merged commit abc8edb into javaparser:master Dec 11, 2022
@jlerbsc jlerbsc added this to the next release milestone Dec 11, 2022
@jlerbsc jlerbsc added the PR: Added A PR that introduces new behaviour (e.g. functionality, tests) label Dec 11, 2022
@jlerbsc
Copy link
Collaborator

jlerbsc commented Dec 11, 2022

Thank you for this contribution.

@4everTheOne 4everTheOne deleted the improve/node-ToAST branch December 12, 2022 08:20
@jlerbsc jlerbsc modified the milestones: next release, 4.0.0 Dec 12, 2022
@jlerbsc jlerbsc modified the milestones: 4.0.0, next release Jan 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Added A PR that introduces new behaviour (e.g. functionality, tests)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants