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

Merge JCore inside the generator #11694

Merged
merged 1,787 commits into from
Jul 5, 2020
Merged

Merge JCore inside the generator #11694

merged 1,787 commits into from
Jul 5, 2020

Conversation

MathieuAA
Copy link
Member

@MathieuAA MathieuAA commented May 4, 2020

What it does:

  • Adds the commits from JCore into this repo
  • Moves the relevant files to a folder named jdl inside the repo
    • does the same for tests
  • Renames the files to match the naming convention here
  • Updates the imported tests
  • Updates the test:unit command in the package.json file to use the mocha config file
    • results in a cleaner package.json "scripts" section

What it doesn't do:

  • Include breaking changes, or any change to existing files
  • Refactor the already present files to use the newer files (another PR is coming, later)
  • Remove JCore as a dependency just yet (another PR is coming, later)

Next steps:

  • Archive the JCore repository
    • ping PR submitters to head over here for their work
  • Do what hasn't been done earlier: refactoring :)

MathieuAA and others added 30 commits December 17, 2019 17:24
Did it so that the DocumentParser was more modular. It has been further
refactored.
To make the DocParser more modular.
Refactored the DocParser in consequence.
To make the DocParser more modular
This is the preferred way of passing a parsed JDL content.
Previously, it was the "document" attribute, but that was too vague a name.
The document attribute is still accepted but will be removed in the future.
Also refactored the doc accordingly
The options field of the JDL entity wasn't used anywhere
What I did:
  - Modified to way entities are converted in the DocParser by splitting the
    entity creation and the entity's fields setting. That gave me a better
    view of an "private access" issue: the DocParser doesn't have to know how
    the JDLEntity stores the fields.
  - Added a setField method to the JDLEntity (takes an iterable) to avoid
    open/close violations
  - Doing the two things enables me to write an EntityConverter, almost
  - Extracted the entity option setting from the loop!
  - To make creating an EntityConverter easier
  - By extracting it to another function, I could see a nested loop and a
    potential refactoring.
commit 002941f1108e5c3171d9745f775b553ae7b11e34
Author: Mathieu Abou-Aichi <mathieu.aa@protonmail.com>
Date:   Thu Dec 26 12:37:59 2019 +0100

    Made option name & value attributes be the same

    For: annotations & relationship options
    Next step: figure out if options can be set to entities, fields, relationships,
    etc. without making a mess in the parsing system and JDL objects

commit e9eed065acfbdd29506a016184635f9efc582af9
Author: Mathieu Abou-Aichi <mathieu.aa@protonmail.com>
Date:   Thu Dec 26 11:39:00 2019 +0100

    Implemented the EntityConverter

    To make the DocParser more modular
    Also renamed and sorted functions for clarity

commit aca9ac6e6ac57a8a38c6602ebdc26fec26fa2298
Author: Mathieu Abou-Aichi <mathieu.aa@protonmail.com>
Date:   Thu Dec 26 10:51:49 2019 +0100

    Renamed function
To replace the Abstract one and the 4 concrete ones
The idea behind this new design:
  - only the config changes depending on the type
  - there's no shared field between the classes (so this isn't a good fit for inheritance)

Refactored the project to create JDLApplications instead
…pp factory

Replaced the previous JDL application classes by the single JDLApplication.
The goal is to make changes easier to do, and reduce complexity as inheritance
made changes delicate and in more than one file.
Now, for each application type, a function exists to get the correct config as
well as default configuration.
Refactored the impacted files to make it work again.
The correct way to create an application now is to:
  - either provide a full configuration to the JDLApplication
  - or use the JDL app factory and provide custom options

Removed:
  - AbstractJDLApplication and its 4 concrete classes
    - Poor design choices as inheritance wasn't the best choice (see previous commit for more info)
@MathieuAA MathieuAA marked this pull request as ready for review July 4, 2020 15:46
@MathieuAA
Copy link
Member Author

Alright! as v7 is the next step, this PR is no longer a draft.

@pascalgrimaud
Copy link
Member

I'd like to merge this, before going further in v7
Is it ready for beeing merged @MathieuAA ?

@MathieuAA
Copy link
Member Author

Unless someone wants to review it first, yes

@pascalgrimaud
Copy link
Member

As mentionned, no breaking changes here, as it's the merge of 2 projects
Then let's go !

@pascalgrimaud pascalgrimaud merged commit 8a93955 into jhipster:master Jul 5, 2020
@MathieuAA
Copy link
Member Author

I don't know if you can do that @pascalgrimaud but now that the merge is live, we should archive JCore

@pascalgrimaud
Copy link
Member

@MathieuAA : I'd prefer to keep jhipster-core like this, and archive it once v7 is released. The reason is, in case we find a bug and we need to do new release of jhipster-core and new patch release of generator-jhipster

@pascalgrimaud pascalgrimaud added this to the 7.0.0 milestone Jul 20, 2020
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.

9 participants