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

Added application generation support for the JDL based on JCore v2 #7339

Merged
merged 3 commits into from Mar 27, 2018
Merged

Added application generation support for the JDL based on JCore v2 #7339

merged 3 commits into from Mar 27, 2018

Conversation

MathieuAA
Copy link
Member

@MathieuAA MathieuAA commented Mar 21, 2018

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

  • Travis tests are green

  • Tests are added where necessary

  • Documentation is added/updated where necessary

  • Coding Rules & Commit Guidelines as per our CONTRIBUTING.md document are followed

Notice the subtle bump from v1.4.6 to v2.0.1 ;)

@DanielFran
Copy link
Member

DanielFran commented Mar 21, 2018

@MathieuAA will you create a PR to update/add jdl files in project JDL samples?

@MathieuAA
Copy link
Member Author

@DanielFran yes :)

@MathieuAA
Copy link
Member Author

MathieuAA commented Mar 21, 2018

@deepu105 I was wondering if the way to generate an app is as simple as generating an entity... From the code, it looks like it's really easy but I'd rather have your opinion before committing the code.

@jdubois
Copy link
Member

jdubois commented Mar 21, 2018

@MathieuAA have you also looked for upgrading https://github.com/jhipster/jdl-studio ?

@MathieuAA
Copy link
Member Author

MathieuAA commented Mar 21, 2018

@jdubois Not yet

edit: just tried, can't install the project for some annoying reason... Will try tomorrow.

@deepu105
Copy link
Member

@MathieuAA give me some time, i'll take a look mostly this weekend.

Copy link
Member

@deepu105 deepu105 left a comment

Choose a reason for hiding this comment

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

Looks fine, from a high level look, will test some edge cases and take a closer look over the weekend

@deepu105
Copy link
Member

@deepu105 I was wondering if the way to generate an app is as simple as generating an entity... From the code, it looks like it's really easy but I'd rather have your opinion before committing the code.

@MathieuAA I didn't understand this comment. Your PR already creates the application from JDL right?

@MathieuAA
Copy link
Member Author

@deepu105 it creates the required .yo-rc.json files and folders (and generates .jhipster + JSON entity files if needed). It doesn't call the app subgen, that was my question :)

@jdubois
Copy link
Member

jdubois commented Mar 22, 2018

This is good for me -> @deepu105 let's wait until your review this week-end

@deepu105
Copy link
Member

@MathieuAA thats easy in the import-jdl index.js new line 156 where we call this.getExistingEntities() just add this inside the try block

if (Object.keys(jdlObject.applications).length !== 0) {
     this.composeWith(require.resolve('../app'), {
           force: this.options.force,
           debug: this.options.debug,
           'skip-install': this.options['skip-install'],
           ... other flags that needs to be passed
     });
}
this.getExistingEntities().forEach((entity) => {
...
});

also, ensure to skip this.rebuildClient(); in the end if (Object.keys(jdlObject.applications).length !== 0) to avoid double webpack build

@MathieuAA
Copy link
Member Author

MathieuAA commented Mar 22, 2018 via email

@deepu105
Copy link
Member

@MathieuAA I didn't do fresh install recently, i can try this weekend. Btw use NPM as it doesnt work well with YARN due to flat structuring

@MathieuAA
Copy link
Member Author

MathieuAA commented Mar 22, 2018 via email

}
} catch (e) {
this.debug('Error:', e);
Copy link
Member

Choose a reason for hiding this comment

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

this was added to help errors be more understandable to end users, this.error(Error while parsing applications and entities from the JDL ${error}.); is not very helpful all the time, so can you retain these, please

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do

}
}
};
}

end() {
if (!this.options['skip-install'] && !this.skipClient && !this.options['json-only']) {
if (!this.options['skip-install'] && !this.skipClient && !this.options['json-only']
&& Object.keys(this.jdlObject.applications).length === 0) {
Copy link
Member

Choose a reason for hiding this comment

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

this can be !shouldGenerateApplications

Copy link
Member Author

Choose a reason for hiding this comment

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

Well spotted!

@jdubois
Copy link
Member

jdubois commented Mar 27, 2018

I generated several applications, including some pretty complex ones, everything looked perfect to me!
So I'm merging this -> @MathieuAA can you still correct later @deepu105 comments about this.debug('Error:', e);?

@jdubois jdubois merged commit 21c9520 into jhipster:master Mar 27, 2018
@MathieuAA
Copy link
Member Author

@jdubois it is fixed, but as I've changed the code GH's diff didn't pick it up...

@DanielFran
Copy link
Member

DanielFran commented Mar 30, 2018

@MathieuAA , sorry to remind you but it is missing update/add jdl files in project JDL samples...

I just think it is impportant to have good jdl examples for testing. ;)

@MathieuAA
Copy link
Member Author

MathieuAA commented Mar 31, 2018 via email

@jdubois jdubois added this to the 5.0.0-beta.0 milestone Apr 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants