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

use constants in generator docker #14563

Merged
merged 13 commits into from
Apr 9, 2021
Merged

use constants in generator docker #14563

merged 13 commits into from
Apr 9, 2021

Conversation

Tcharl
Copy link
Contributor

@Tcharl Tcharl commented Apr 2, 2021

contributes to generator understanding

Use constants in generator docker

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.

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.

Would be nice if you limit PRs to ~20 files.
It will be a lot easier.

generators/entity-client/index.js Outdated Show resolved Hide resolved
generators/entity-client/index.js Outdated Show resolved Hide resolved
@Tcharl Tcharl force-pushed the constantref branch 2 times, most recently from 7f20753 to 2feac12 Compare April 6, 2021 10:40
@Tcharl
Copy link
Contributor Author

Tcharl commented Apr 6, 2021

@mshima better now? FYI, I removed the elasticSearchFalse in favour of testing elasticSearch as it is set to false if disabled

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.

The entity* generators needs more work at derived properties due to Microservice support.
Some config is loaded from the gateway, others from Microservice.
Others seems ok.

IMO you should remove derived properties from entities at this PR, just use constants.
Microservice support is really complicated.

generators/docker-compose/index.js Outdated Show resolved Hide resolved
generators/generator-base-docker.js Show resolved Hide resolved
generators/docker-compose/index.js Show resolved Hide resolved
generators/entity/index.js Outdated Show resolved Hide resolved
generators/generator-base.js Outdated Show resolved Hide resolved
generators/entity/index.js Outdated Show resolved Hide resolved
generators/generator-base.js Outdated Show resolved Hide resolved
@Tcharl Tcharl force-pushed the constantref branch 2 times, most recently from fb3a495 to 3a76bd3 Compare April 6, 2021 21:11
@Tcharl
Copy link
Contributor Author

Tcharl commented Apr 6, 2021

This entity-client generator's code is crazy: so much nifty details everywhere...

@Tcharl Tcharl requested a review from mshima April 7, 2021 16:01
@Tcharl
Copy link
Contributor Author

Tcharl commented Apr 8, 2021

@mshima or @pascalgrimaud can you take a look and maybe quick (as any modification on the angular entity part will lead to a conflict)? I know it's tough and I'm sorry for that, but I think I found the elegant way to achieve it and now templates are way more clean.
I would like to continue on that refactoring to be globally applied and I guess we'll then have a way more maintainable codebase.

I guess that the overall refacto will need two or three more full refinement but as it will only be at the js level after the first pass we'll be able to make something way more scalable and modular (I can't wait to split up that 2.6 thousand-line BaseGenerator class :-p)

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.

@Tcharl last commit is not ok, or it fixes bugs, not sure. The generated code is not identical anymore.

While testing, I've realized that micro service support was already buggy for entity due to a racing condition. We will fix this later.

Revert last commit and merge.
Create a new PR with the last commit.

generators/entity/index.js Show resolved Hide resolved
@mshima
Copy link
Member

mshima commented Apr 8, 2021

@Tcharl I've just merged #14561 and realized that I've replaced builtToolUndefined with buildTookUnknown, that you may be using here.

Feel free to add it again.

@Tcharl
Copy link
Contributor Author

Tcharl commented Apr 8, 2021

Sorry @mshima but I don't understand your remark: should I only accept your change and that's it?
What is different compared to previous generation? It shouldn't have modified anything (maybe some carriage return but should be the only difference).

@mshima
Copy link
Member

mshima commented Apr 8, 2021

Sorry @mshima but I don't understand your remark: should I only accept your change and that's it?

Yes, it stopped recalculating jhiPrefixDashed.

What is different compared to previous generation? It shouldn't have modified anything (maybe some carriage return but should be the only difference).

See the builds bellow.
Builds taking more than 6min means that the source code have changed.
Eg. go to details of Angular / ngx-default (pull_request) at Merge: merge project diff step:

You will see the diff:

@@ -868,6 +868,13 @@
             >
               This field is required.
             </small>
+            <small
+              class="form-text text-danger"
+              [hidden]="!editForm.get('localDateRequiredTom')?.errors?.ZonedDateTimelocal"
+              myPrefixTranslate="entity.validation.ZonedDateTimelocal"
+            >
+              This field should be a date and time.
+            </small>
           </div>
         </div>

Tcharl and others added 3 commits April 8, 2021 19:43
@Tcharl
Copy link
Contributor Author

Tcharl commented Apr 8, 2021

I got the one you mentioned :-) and thank you for the TIPs!. It was a bug on my side (I used Temporal which included ZonedDateTime, LocalDate and Instant instead of Timed, which only includes ZonedDateTime and Instant).

Let's rebuild it and see if things are always modified: fingers crossed! Anyway, the review is still welcome: I'll hope I don't have included other side effect

@mshima mshima closed this Apr 8, 2021
@mshima mshima reopened this Apr 8, 2021
@Tcharl
Copy link
Contributor Author

Tcharl commented Apr 8, 2021

So, good to go? Ready for the next round (I promise, it will be less than 20 files this time ;-))

@pascalgrimaud
Copy link
Member

Go ahead @Tcharl !

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