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

Correct typos #10788

Merged
merged 7 commits into from
May 22, 2020
Merged

Correct typos #10788

merged 7 commits into from
May 22, 2020

Conversation

vivekmore
Copy link
Contributor

@vivekmore vivekmore commented Nov 17, 2019

Copy link
Contributor

@murdos murdos left a comment

Choose a reason for hiding this comment

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

The changes in the app subgenerator and in statistics.js are fine IMO, however I think the changes in client and server subgenerators are breaking changes since blueprints could use it.
These changes could probably be postponed to v7

@vivekmore
Copy link
Contributor Author

Makes sense.
so what do you suggest?
submit PR on separate 7.x branch?

@murdos murdos added the v7 label Nov 17, 2019
@murdos
Copy link
Contributor

murdos commented Nov 17, 2019

Makes sense.
so what do you suggest?
submit PR on separate 7.x branch?

There's no 7.x branch, we only have maintenance branches.
I've added the 7.x label, I guess the PR will stay open until we start to work on 7.x

@CLAassistant
Copy link

CLAassistant commented Apr 20, 2020

CLA assistant check
All committers have signed the CLA.

@jdubois
Copy link
Member

jdubois commented May 20, 2020

Let's first do the "easy" typos, that are not used by the sub-generators. For the others, it's going to be difficult.

@vivekmore
Copy link
Contributor Author

Let's first do the "easy" typos, that are not used by the sub-generators. For the others, it's going to be difficult.

How about a gradual upgrade by keeping both variants like follows:

methodWithoutTypo() {
   ...
   // actual code
   ...
}

methodWITHtypo() {
   this.log.warn('Please use methodWithoutTypo() as this method will be deprecated in next major release');
   methodWithoutTypo();
}

@MathieuAA
Copy link
Member

MathieuAA commented May 20, 2020

@vivekmore that's a good idea! Can you add a comment to the "functions with typos" that says

@deprecated
To be removed for JHipster v7

That way, we could clean them up for the next major


edit I should read everything before commenting, seems like you've got the idea :)

@vivekmore vivekmore requested a review from murdos May 21, 2020 02:25
@murdos
Copy link
Contributor

murdos commented May 21, 2020

Let's first do the "easy" typos, that are not used by the sub-generators. For the others, it's going to be difficult.

These will still be breaking changes, in particular for blueprints.

For example, if a blueprint is overriding a particular step (setupClientconsts in the example below), it won't be overridden anymore if we rename it in main generator to setupClientConstants:

get writing() {
        const phaseFromJHipster = super._writing();
        const customPhaseSteps = {
            setupClientconsts() {
                // Do things that change generator-jhipster behavior...
            },
        };
        return { ...phaseFromJHipster, ...customPhaseSteps };
    }

Development on V7 will start soon, let's wait a bit more and don't break V6 for blueprints (and there're a more and more)

@vivekmore
Copy link
Contributor Author

@murdos - You're right
However, to get this small PR merged I've removed the breaking changes related to setup*() functions.

I'll make another PR as soon as v7 development starts that:

  • removes deprecated methods from this PR
  • corrects the setup*() methods (that I removed from my most recent update to this PR)

does that seem reasonable?

Copy link
Contributor

@murdos murdos left a comment

Choose a reason for hiding this comment

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

I highlighted the 3 remaining breaking changes that should be reverted. Then this PR could be merged ;)

generators/app/index.js Outdated Show resolved Hide resolved
generators/client/index.js Outdated Show resolved Hide resolved
generators/server/index.js Outdated Show resolved Hide resolved
vivekmore and others added 3 commits May 21, 2020 22:51
Co-authored-by: Aurélien Mino <[email protected]>
Co-authored-by: Aurélien Mino <[email protected]>
Co-authored-by: Aurélien Mino <[email protected]>
@vivekmore
Copy link
Contributor Author

I highlighted the 3 remaining breaking changes that should be reverted. Then this PR could be merged ;)

Good catch @murdos ! Sorry for missing these, and thank you for being patient.
I realize I have a few more stuff to learn about things exported from Generators.

@vivekmore vivekmore requested a review from murdos May 22, 2020 03:26
@murdos murdos removed the v7 label May 22, 2020
@murdos murdos merged commit d66bc7a into jhipster:master May 22, 2020
@vivekmore vivekmore deleted the correct-typos branch May 22, 2020 12:48
@pascalgrimaud pascalgrimaud added this to the 6.9.1 milestone Jun 1, 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.

7 participants