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 jdl enum comments #15364

Merged
merged 12 commits into from
Aug 31, 2021
Merged

Conversation

swarajsaaj
Copy link
Contributor

@swarajsaaj swarajsaaj commented Jun 17, 2021


Adds javadocs to Enum types and enum values from JDL.
Related to #14869

Please make sure the below checklist is followed for Pull Requests.

When you are still working on the PR, consider converting it to Draft (bellow reviewers) and adding skip-ci label, you can still see CI build result at your branch.

@swarajsaaj
Copy link
Contributor Author

Can you please have a look over this @MathieuAA whenever you get some time?
Thanks.

@github-actions github-actions bot added the Stale label Jul 29, 2021
@github-actions github-actions bot closed this Aug 5, 2021
@mshima mshima reopened this Aug 5, 2021
@@ -1081,24 +1086,31 @@ entity A {

it('should parse it', () => {
expect(parsedEnum).to.deep.equal({
javadoc: ' country enum ',
Copy link
Member

Choose a reason for hiding this comment

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

I find it weird the comment isn't trimmed, WDYT? I would have expected it to be country enum.

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 tried to keep it in sync with how entity comments are being parsed

javadoc: ' field comment ',
javadoc: ' A comment ',

But I agree there is no use of trailing space, I have now fixed this for entities as well by changing regex in jdl-ast-builder-visitor.js trimComment()

test/jdl/models/jdl-enum.spec.js Outdated Show resolved Hide resolved
swarajsaaj and others added 3 commits August 15, 2021 12:40
Co-authored-by: Mathieu ABOU-AICHI <MathieuAA@users.noreply.github.com>
Trim trailing spaces from JDL parser

Related to jhipster#14869
@@ -513,10 +513,11 @@ function getEnumInfo(field, clientRootFolder) {
const customValuesState = getCustomValuesState(enums);
return {
enumName: fieldType,
javadoc: field.fieldTypeJavadoc && this.getJavadoc(field.fieldTypeJavadoc, 0),
Copy link
Member

Choose a reason for hiding this comment

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

why the 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is the indentSize.
Not sure if this has any effect as overall generated file seems formatted (with prettier?)

Copy link
Member

Choose a reason for hiding this comment

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

you can always test :) try using 2 instead of 0, or see if 0 can be the default

but if I'm not mistaken, the indent level is important

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 just tested :), indent level matters only if we are generating without prettier (i.e. with --skip-prettier). by default it prettifies all the files with proper indentation.
To cover that case I changed the indents to 4 for field javadoc and default to zero for class level javadoc.
Thanks for pointing out.

jdl/models/jdl-enum.js Show resolved Hide resolved
jdl/parsing/jdl-ast-builder-visitor.js Show resolved Hide resolved
test/jdl/grammar/grammar.spec.js Show resolved Hide resolved
…n/enumeration/Enum.java.ejs

Co-authored-by: Mathieu ABOU-AICHI <MathieuAA@users.noreply.github.com>
Copy link
Member

@MathieuAA MathieuAA left a comment

Choose a reason for hiding this comment

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

just one comment which you can resolve if you want, but it's approved! thanks for the work!!

@swarajsaaj
Copy link
Contributor Author

@MathieuAA Thanks for approving, I also added the indentation for cases when prettier is not run (using --skip-prettier) and the test case update for indented javadoc.
Pipeline seems failing due to unrelated cause (maybe a retrigger fixes it)

@DanielFran
Copy link
Member

@swarajsaaj Relaunched failing tests...

Copy link
Member

@mshima mshima left a comment

Choose a reason for hiding this comment

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

Ident adjusts

@DanielFran
Copy link
Member

Let's merge this since @mshima fixed lint issues

@DanielFran DanielFran merged commit c9b4cc0 into jhipster:main Aug 31, 2021
@DanielFran
Copy link
Member

Thanks @swarajsaaj for the contribution

@swarajsaaj
Copy link
Contributor Author

Thanks @DanielFran for merging and committing the suggestions.
Thanks @MathieuAA and @mshima for reviews.

@pascalgrimaud pascalgrimaud added this to the 7.2.0 milestone Sep 11, 2021
@pascalgrimaud pascalgrimaud added $$ bug-bounty $$ https://www.jhipster.tech/bug-bounties/ $300 https://www.jhipster.tech/bug-bounties/ labels Sep 11, 2021
@pascalgrimaud
Copy link
Member

I was on vacations and didn't manage to follow everything.
There is a lot of work done here, so it deserves a bounty.

Plz claim it @swarajsaaj

@swarajsaaj
Copy link
Contributor Author

Thanks @pascalgrimaud 😄 , i claim the bounty at at https://opencollective.com/generator-jhipster/expenses/49963

@pascalgrimaud
Copy link
Member

@swarajsaaj : approved :-) thanks for your hard work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: enhancement 🔧 $$ bug-bounty $$ https://www.jhipster.tech/bug-bounties/ theme: JDL $300 https://www.jhipster.tech/bug-bounties/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants