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

add Java 17 sealed/non-sealed classes #3997

Merged
merged 2 commits into from
Apr 19, 2023
Merged

add Java 17 sealed/non-sealed classes #3997

merged 2 commits into from
Apr 19, 2023

Conversation

kris-scheibe
Copy link
Contributor

This adds sealed/non-sealed classes, which were fully added in Java 17, see #2888

@jlerbsc
Copy link
Collaborator

jlerbsc commented Apr 18, 2023

Thank you for this suggestion which I will look into very soon.

@@ -148,6 +156,20 @@ public ClassOrInterfaceDeclaration setImplementedTypes(final NodeList<ClassOrInt
return this;
}

@Generated("com.github.javaparser.generator.core.node.PropertyGenerator")
public ClassOrInterfaceDeclaration setPermittedTypes(final NodeList<ClassOrInterfaceType> permittedTypes) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you confirm for me that the generated properties are actually generated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did run the metamodel and core generator scripts, which added the getters/setters after I added the field. But that didn't modify the @AllFieldsConstructor-constructor or the main-constructor. Those I had to update manually.

Also the generator updated pretty much all files, mostly by adding/removing whitespace in imports and such. So I only kept the changes related to the sealed/non-sealed logic and reverted everything else.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I admit that I don't know this part very well (generator) but what seems curious to me, if I understand your answer correctly, is that the generator generates the following code but does not generate the property "permittedTypes".

this.permittedTypes.setParentNode(null);

For my information, how did you know you had to run the generators and manipulate the metamodel? Did you consult any documentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't find any documentation for the generator (only IDE-setup, coding-guidelines, etc.). After adding the permittedTypes field I just looked around how to invoke the generator for the accessors and other functions. This lead me to the javaparser-core-generators module, the run-generators Maven-profile and finally the run_core_generators.sh script, which contains the comment

# Runs all the code generators.
# If the node structure was changed, run_metamodel_generator.sh first!

This is essentially the "documentation" to first run the metamodel-generator and then the code-generator. (Plus a bit of inductive reasoning, as I've seen similar projects and code-generators before.)

So, yes, after adding the permittedTypes field, running the generators generated the matching accessors and other utility-functions in ClassOrInterfaceDeclaration, updated various visitor- and observer-classes, and the metamodel to each include the new field.
I'm not sure, why the MainConstructorGenerator didn't update the "main constructor" properly, as I had to update this one manually.


Also the generators changed the formatting of all files, that contained generated code by stripping out empty lines in the import statements, so from:

import com.github.javaparser.resolution.Resolvable;
import com.github.javaparser.resolution.declarations.ResolvedReferenceTypeDeclaration;

import java.util.Optional;
import java.util.function.Consumer;

import static com.github.javaparser.utils.Utils.assertNotNull;

to

import com.github.javaparser.resolution.Resolvable;
import com.github.javaparser.resolution.declarations.ResolvedReferenceTypeDeclaration;
import java.util.Optional;
import java.util.function.Consumer;
import static com.github.javaparser.utils.Utils.assertNotNull;

I reverted these changes, as I didn't want to inflate the PR with hundreds of reformatted, but unrelated, java files.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The only documentation I found on this topic is here. Your suggestions for improving it are welcome. https://javaparser.org/code-generation-and-maven-in-javaparser/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did look on the javaparser website, specifically https://javaparser.org/contributing.html , but this only links back to various github pages.
Honestly, without knowing that the code-generator exists I wouldn't have looked outside the contributing-section.

The contributing-section links to https://github.com/javaparser/javaparser/blob/master/CONTRIBUTING.md, which has links to the github wiki. IMO, the wiki is the correct place to document how to use javaparser and how to contribute. Or at least there should be pages in the wiki that link back to specific javaparser.org blog-articles, so that users/contributers can find the information, without already knowing where to look.

The blog just isn't a good place for technical documentation, but I also don't know the "official" status of javaparser.org. To me a lot of information and documentation seems to be spread across the javaparser.org and tomassetti.me website/blog and the github wiki and documents, which makes it hard to find specific documentation.

@codecov
Copy link

codecov bot commented Apr 18, 2023

Codecov Report

Merging #3997 (4bc5ea3) into master (fdbeb89) will increase coverage by 0.004%.
The diff coverage is 86.904%.

Impacted file tree graph

@@               Coverage Diff               @@
##              master     #3997       +/-   ##
===============================================
+ Coverage     57.806%   57.811%   +0.004%     
  Complexity      2295      2295               
===============================================
  Files            642       642               
  Lines          34233     34284       +51     
  Branches        5926      5936       +10     
===============================================
+ Hits           19789     19820       +31     
- Misses         12322     12336       +14     
- Partials        2122      2128        +6     
Flag Coverage Δ
AlsoSlowTests 57.811% <86.904%> (+0.004%) ⬆️
javaparser-core 50.358% <86.309%> (+0.019%) ⬆️
javaparser-symbol-solver 36.996% <16.666%> (+<0.001%) ⬆️
jdk-10 57.808% <86.904%> (+0.007%) ⬆️
jdk-11 57.808% <86.904%> (+0.004%) ⬆️
jdk-12 57.808% <86.904%> (+0.004%) ⬆️
jdk-13 57.808% <86.904%> (+0.004%) ⬆️
jdk-14 57.808% <86.904%> (+0.004%) ⬆️
jdk-15 57.808% <86.904%> (+0.004%) ⬆️
jdk-16 57.808% <86.904%> (+0.013%) ⬆️
jdk-8 57.810% <86.904%> (+0.010%) ⬆️
jdk-9 57.808% <86.904%> (+0.007%) ⬆️
macos-latest 57.805% <86.904%> (+0.004%) ⬆️
ubuntu-latest 57.799% <86.904%> (+0.004%) ⬆️
windows-latest 57.793% <86.904%> (+0.004%) ⬆️

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

Impacted Files Coverage Δ
...rc/main/java/com/github/javaparser/TokenTypes.java 84.615% <ø> (ø)
...m/github/javaparser/ast/visitor/EqualsVisitor.java 28.199% <0.000%> (-0.062%) ⬇️
...aparser/ast/visitor/GenericListVisitorAdapter.java 0.000% <0.000%> (ø)
...javaparser/ast/visitor/NoCommentEqualsVisitor.java 3.724% <0.000%> (-0.011%) ⬇️
...vaparser/ast/visitor/NoCommentHashCodeVisitor.java 68.224% <0.000%> (ø)
...etamodel/ClassOrInterfaceDeclarationMetaModel.java 100.000% <ø> (ø)
.../javaparser/ast/visitor/GenericVisitorAdapter.java 9.762% <33.333%> (+0.060%) ⬆️
...vaparser/ast/body/ClassOrInterfaceDeclaration.java 57.894% <50.000%> (-2.106%) ⬇️
...r/language_level_validations/Java1_0Validator.java 96.923% <75.000%> (-1.438%) ⬇️
...src/main/java/com/github/javaparser/JavaToken.java 92.359% <100.000%> (+0.104%) ⬆️
... and 9 more

Continue to review full report in Codecov by Sentry.

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

@jlerbsc
Copy link
Collaborator

jlerbsc commented Apr 19, 2023

If you have any other suggestions, feel free to submit them. For example, there is support for local interfaces/enumerations... (java16) or certain specific features provided by java18 and java19.

@jlerbsc jlerbsc merged commit 6902784 into javaparser:master Apr 19, 2023
31 checks passed
@jlerbsc
Copy link
Collaborator

jlerbsc commented Apr 19, 2023

Although I don't know this part very well it seems to me that this PR is correct. However, it remains to be ensured that the Lexical Preserving Printer and the Symbol Solver also meet expectations. Thank you for your involvement.

@jlerbsc jlerbsc added this to the next release milestone Apr 19, 2023
@jlerbsc jlerbsc added the PR: Added A PR that introduces new behaviour (e.g. functionality, tests) label Apr 19, 2023
@kris-scheibe
Copy link
Contributor Author

At work we use javaparser as a code-generator, but we only recently transitioned from Java 11 to Java 17 and haven't really started using the new language-features yet. But I can have a look into new language-features, especially as the next LTS-version will be released this fall.

@jlerbsc are you OK with a PR, that only changes the formatting to what the code-generators generate now? This would make future contributions much easier! Ideally running the code-generator on a freshly checked-out project shouldn't change anything, but this isn't the case right now.

@kris-scheibe kris-scheibe deleted the feature/java17-sealed-classes branch April 19, 2023 10:32
@jlerbsc
Copy link
Collaborator

jlerbsc commented Apr 19, 2023

Yes of course. Personally I have never started it. I know old contributors did this at least once a year.
To be completely honest, I am "the last of the mohicans" the previous historical contributors no longer contribute to the project and I try to maintain it. New contributors would be welcome. Out of curiosity, what company do you work for?

@kris-scheibe
Copy link
Contributor Author

Thank you for keeping the project alive! I know that being a maintainer of an OSS-project can be a thankless job.
I'd be happy to contribute to this project -- at least to the parser (I haven't really worked much with the symbol-solver, but the parser itself seems pretty straight-forward).

I work for a beauty-retailer in Germany. I'm not going to name my employer here, as we don't have an official policy regarding contributions to OSS-projects. In the past, direct contributions have just been the fastest way to get bugs fixed and features added.
Also I like to contribute back as projects like this one in my free time as it makes my work so much easier!

@blacelle
Copy link
Contributor

Hello. I feel this is missing from https://github.com/javaparser/javaparser/blob/master/changelog.md

It impacted us (positively, but unexpectedly) in diffplug/spotless#1725

@jlerbsc
Copy link
Collaborator

jlerbsc commented May 25, 2023

No, the change log is updated automatically when we publish the project. What you see in Next Release is just a template.

@blacelle
Copy link
Contributor

blacelle commented May 25, 2023

No ? Where is this (#3997, or a ref to sealed keyword) referred in the changelog then? I believe this is available in JavaParser 3.25.3

@jlerbsc
Copy link
Collaborator

jlerbsc commented May 25, 2023

Yes this PR is merged since 3.25.3 but the changelog does not mention it. It is a mistake. The change log is updated automatically so i don't know why there is an issue.

@Auties00
Copy link
Contributor

Is there any reason why com.github.javaparser.ast.Modifer doesn't provide a named constructor for the SEALED and NON_SEALED modifiers? Every other modifier is covered as far as I can tell. I can open a quick PR if it was missed here for whatever reason

@jlerbsc
Copy link
Collaborator

jlerbsc commented Jun 18, 2023

Thank you for your improvement suggestion.

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.

None yet

4 participants