precommit in master branch

[Matt Raible]

Hello everyone,

What is the point of this?

husky > npm run -s precommit (node v9.5.0)

sh: lint-staged: command not found

husky > pre-commit hook failed (add --no-verify to bypass)

[Deepu K Sasidharan]

Hi Matt,

We don't have pre commit in the generator, but we have it in the generated project. It is used by prettier to format code when you commit them. I guess you are seeing this since you might have generated a project inside the travis samples. And since there is parent git folder the precommit hook is being added there. I guess if you run the cleanup command for the travis build it would be fine, else we can also add a flag and use that to skip this step when generating sample apps

--
You received this message because you are subscribed to the Google Groups "JHipster dev team" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jhipster-dev...@googlegroups.com.
To post to this group, send email to jhipst...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jhipster-dev/6DEA9298-342D-4375-989A-C087038D83C8%40raibledesigns.com.
For more options, visit https://groups.google.com/d/optout.

[Julien Dubois]

BTW I have mixed feelings about our pre-commit hook.

- We could totally embrace this, and do more than just run prettier

- BUT people might not like that we change their code, and annoy them when they want to commit

-> I'm not sure what the right approach is

2018-03-07 7:39 GMT+01:00 Deepu K Sasidharan <d4u...@gmail.com>:

Hi Matt,

We don't have pre commit in the generator, but we have it in the generated project. It is used by prettier to format code when you commit them. I guess you are seeing this since you might have generated a project inside the travis samples. And since there is parent git folder the precommit hook is being added there. I guess if you run the cleanup command for the travis build it would be fine, else we can also add a flag and use that to skip this step when generating sample apps

On Wed, 7 Mar 2018, 4:33 am Matt Raible, <ma...@raibledesigns.com> wrote:

Hello everyone,

What is the point of this?

husky > npm run -s precommit (node v9.5.0)

sh: lint-staged: command not found

husky > pre-commit hook failed (add --no-verify to bypass)

--
You received this message because you are subscribed to the Google Groups "JHipster dev team" group.

To unsubscribe from this group and stop receiving emails from it, send an email to jhipster-dev+unsubscribe@googlegroups.com.

To post to this group, send email to jhipst...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jhipster-dev/6DEA9298-342D-4375-989A-C087038D83C8%40raibledesigns.com.
For more options, visit https://groups.google.com/d/optout.

--

You received this message because you are subscribed to the Google Groups "JHipster dev team" group.

To unsubscribe from this group and stop receiving emails from it, send an email to jhipster-dev+unsubscribe@googlegroups.com.

To post to this group, send email to jhipst...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/jhipster-dev/CAFXry562wL2YiutXh%3DBm38_DPVrbotriHgB3KNUgdVrEL7XvNg%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--

Julien Dubois

Twitter: @juliendubois

[Deepu K Sasidharan]

I'm using this in our company and so far people are happy with it. We could add a flag or docs to explain how to disable it. Without a pre commit hook adding prettier doesn't make much sense.

On Wed, 7 Mar 2018, 7:57 am Julien Dubois, <julien...@gmail.com> wrote:

BTW I have mixed feelings about our pre-commit hook.

- We could totally embrace this, and do more than just run prettier

- BUT people might not like that we change their code, and annoy them when they want to commit

-> I'm not sure what the right approach is

2018-03-07 7:39 GMT+01:00 Deepu K Sasidharan <d4u...@gmail.com>:

Hi Matt,

We don't have pre commit in the generator, but we have it in the generated project. It is used by prettier to format code when you commit them. I guess you are seeing this since you might have generated a project inside the travis samples. And since there is parent git folder the precommit hook is being added there. I guess if you run the cleanup command for the travis build it would be fine, else we can also add a flag and use that to skip this step when generating sample apps

On Wed, 7 Mar 2018, 4:33 am Matt Raible, <ma...@raibledesigns.com> wrote:

Hello everyone,

What is the point of this?

husky > npm run -s precommit (node v9.5.0)

sh: lint-staged: command not found

husky > pre-commit hook failed (add --no-verify to bypass)

--
You received this message because you are subscribed to the Google Groups "JHipster dev team" group.

To unsubscribe from this group and stop receiving emails from it, send an email to jhipster-dev...@googlegroups.com.

To post to this group, send email to jhipst...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jhipster-dev/6DEA9298-342D-4375-989A-C087038D83C8%40raibledesigns.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "JHipster dev team" group.

To unsubscribe from this group and stop receiving emails from it, send an email to jhipster-dev...@googlegroups.com.

To post to this group, send email to jhipst...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/jhipster-dev/CAFXry562wL2YiutXh%3DBm38_DPVrbotriHgB3KNUgdVrEL7XvNg%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

[Gaël Marziou]

but prettier is also launched by generator.

Isn't it enough?

[Pierre BESSON]

We are also using precommit hooks with husky on our angularjs 1 app and having great success with it. Also with prettier it's a must have as it enforces that prettier is run before anything is commited. Big +1 to keep it.

2018-03-07 9:28 GMT+01:00 Gaël Marziou <gael.m...@gmail.com>:

but prettier is also launched by generator.

Isn't it enough?

--

You received this message because you are subscribed to the Google Groups "JHipster dev team" group.

To unsubscribe from this group and stop receiving emails from it, send an email to jhipster-dev+unsubscribe@googlegroups.com.

To post to this group, send email to jhipst...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/jhipster-dev/763a1072-095d-4286-9438-63d182a056f4%40googlegroups.com.

[Julien Dubois]

OK let's keep it - we'll have the beta releases to see if that's good, anyway

I'll try to do the first beta release tomorrow

To view this discussion on the web visit https://groups.google.com/d/msgid/jhipster-dev/CAL_TkmkBWXSOHWsvboG8R-H9iwuja3TtrCgrOcCgBFyFjFtUSQ%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

[Gaël Marziou]

Personally, I would disable it on my projects because I don't want it to modify my business classes which are totally unrelated with JHipster and classes that are generated by other code generators like wsdl2java.

This why I'd like to restrict prettier to run on code generated by JHipster, I have started looking at Yeoman transform streams.

[Deepu K Sasidharan]

Gael, prettier now transforms any code that is .ts or .tsx, so your java code should be fine. If you generate any typescript code using other tools, IMO it makes sense to have same formatting for all files of a specific technology. And without a precommit, you cant ensure that for new code and updates. I'll gladly add a flag if you want to supress the precommit hook, or just remove it from the package.json.

Thanks & Regards,

Deepu

On Wed, Mar 7, 2018 at 2:04 PM, Gaël Marziou <gael.m...@gmail.com> wrote:

Personally, I would disable it on my projects because I don't want it to modify my business classes which are totally unrelated with JHipster and classes that are generated by other code generators like wsdl2java.

This why I'd like to restrict prettier to run on code generated by JHipster, I have started looking at Yeoman transform streams.

--
You received this message because you are subscribed to the Google Groups "JHipster dev team" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jhipster-dev+unsubscribe@googlegroups.com.
To post to this group, send email to jhipst...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/jhipster-dev/94dd3ab1-5984-41fa-aae2-bfa547bba319%40googlegroups.com.

[Matt Raible]

I see it when I try to commit code in generator-jhipster. Are you saying this is something I’ve done locally and it shouldn’t be happening?

[Deepu K Sasidharan]

Yes, did you create any sample project under the travis/samples folder? can you try to run ` ./travis/build-samples.sh clean` under the generator folder

Thanks & Regards,

Deepu

On Wed, Mar 7, 2018 at 3:03 PM, Matt Raible <ma...@raibledesigns.com> wrote:

I see it when I try to commit code in generator-jhipster. Are you saying this is something I’ve done locally and it shouldn’t be happening?

On Mar 6, 2018, at 11:39 PM, Deepu K Sasidharan <d4u...@gmail.com> wrote:

Hi Matt,

We don't have pre commit in the generator, but we have it in the generated project. It is used by prettier to format code when you commit them. I guess you are seeing this since you might have generated a project inside the travis samples. And since there is parent git folder the precommit hook is being added there. I guess if you run the cleanup command for the travis build it would be fine, else we can also add a flag and use that to skip this step when generating sample apps

On Wed, 7 Mar 2018, 4:33 am Matt Raible, <ma...@raibledesigns.com> wrote:

Hello everyone,

What is the point of this?

husky > npm run -s precommit (node v9.5.0)

sh: lint-staged: command not found

husky > pre-commit hook failed (add --no-verify to bypass)

--
You received this message because you are subscribed to the Google Groups "JHipster dev team" group.

To unsubscribe from this group and stop receiving emails from it, send an email to jhipster-dev+unsubscribe@googlegroups.com.

[Matt Raible]

I did, that’s probably it. Thanks.

Personally, I get annoyed by precommits and find they add friction to the commit process.

[Julien Dubois]

Yes, next step will add lint, and then tests, and then commiting will take 2 hours, and then nobody will ever commit anything :-)

To view this discussion on the web visit https://groups.google.com/d/msgid/jhipster-dev/1E5FE14B-F253-4EF9-B50B-6B2FD7C35F4F%40raibledesigns.com.

For more options, visit https://groups.google.com/d/optout.

[Deepu K Sasidharan]

Ok I'll add a flag to disable it. But then if you disable it, you won't be making use of prettier :)

On Thu, 8 Mar 2018, 7:38 am Julien Dubois, <julien...@gmail.com> wrote:

Yes, next step will add lint, and then tests, and then commiting will take 2 hours, and then nobody will ever commit anything :-)

2018-03-08 0:30 GMT+01:00 Matt Raible <ma...@raibledesigns.com>:

I did, that’s probably it. Thanks.

Personally, I get annoyed by precommits and find they add friction to the commit process.

On Mar 7, 2018, at 1:07 PM, Deepu K Sasidharan <d4u...@gmail.com> wrote:

Yes, did you create any sample project under the travis/samples folder? can you try to run ` ./travis/build-samples.sh clean` under the generator folder

Thanks & Regards,

Deepu

On Wed, Mar 7, 2018 at 3:03 PM, Matt Raible <ma...@raibledesigns.com> wrote:

I see it when I try to commit code in generator-jhipster. Are you saying this is something I’ve done locally and it shouldn’t be happening?

On Mar 6, 2018, at 11:39 PM, Deepu K Sasidharan <d4u...@gmail.com> wrote:

Hi Matt,

We don't have pre commit in the generator, but we have it in the generated project. It is used by prettier to format code when you commit them. I guess you are seeing this since you might have generated a project inside the travis samples. And since there is parent git folder the precommit hook is being added there. I guess if you run the cleanup command for the travis build it would be fine, else we can also add a flag and use that to skip this step when generating sample apps

On Wed, 7 Mar 2018, 4:33 am Matt Raible, <ma...@raibledesigns.com> wrote:

Hello everyone,

What is the point of this?

husky > npm run -s precommit (node v9.5.0)

sh: lint-staged: command not found

husky > pre-commit hook failed (add --no-verify to bypass)

--
You received this message because you are subscribed to the Google Groups "JHipster dev team" group.

To unsubscribe from this group and stop receiving emails from it, send an email to jhipster-dev...@googlegroups.com.

To post to this group, send email to jhipst...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jhipster-dev/6DEA9298-342D-4375-989A-C087038D83C8%40raibledesigns.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "JHipster dev team" group.

To unsubscribe from this group and stop receiving emails from it, send an email to jhipster-dev...@googlegroups.com.

To post to this group, send email to jhipst...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jhipster-dev/1E5FE14B-F253-4EF9-B50B-6B2FD7C35F4F%40raibledesigns.com.
For more options, visit https://groups.google.com/d/optout.

[Gaël Marziou]

A flag is good but the problem is that the default value is to add this hook.

This feature uses husky which activates on yarn/npm install, which means it is easier for a user to add it afterwards

What I found from little research let me think it could be not safe to be that intrusive:

- several issues opened for integration with other tools: intellij, vs code, git kraken

https://github.com/typicode/husky/issues

- it uses npm even if yarn is configured in JHipster project

- I don't know what will happen in case of a project upgrade which has already git hooks, will husky overwrite them, see https://github.com/typicode/husky/issues/36

- it supports Windows since last month only

- we should add a `--no-verify` flag to all git commit calls in jhipster upgrade as we don't want it to fail due to prettier

[Pascal GRIMAUD]

Maybe we should do a vote for that?

--
You received this message because you are subscribed to the Google Groups "JHipster dev team" group.

To unsubscribe from this group and stop receiving emails from it, send an email to jhipster-dev+unsubscribe@googlegroups.com.

To post to this group, send email to jhipst...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/jhipster-dev/166e8b86-5873-4c5a-97fd-5f8912397fc2%40googlegroups.com.

[Deepu K Sasidharan]

Yes, we could vote on it, but here are some of my thoughts.

1. Prettier is useless without a pre-commit hook, and I would rather not add prettier at all if there is no pre-commit hook. As it will lead to code with incorrect formatting as we disabled rules in tslint which are conflicting. Also, what is the point of having it if you are not using it? so if disabling I would suggest that we use it by default as transform stream(what Gael is working on) then add a prettier as an option in other technology or new prompt. If prettier is not selected, add back rules in tslint as we had earlier else disable it.
2. I'm using it on my projects at work, and so far nobody had an issue with IntelliJ, Vscode etc. And I really find it useful. Its also something which is widely used especially in the react community.
   

Thanks & Regards,

Deepu

To view this discussion on the web visit https://groups.google.com/d/msgid/jhipster-dev/CALUG8Vuhw_TSciN7tYQUbWm5vuwGX%2B6sYpj5eQb3wd2uq2_24Q%40mail.gmail.com.

[Gaël Marziou]

Prettier is useful in 2 cases:

1. JHipster team so that we don't have to care too much about formatting in EJS templates

2. JHipster users in their generated projects

No question about #1, we need it and it's covered by transform streams.

My point about #2 is that we should let the users decide because it could conflict with other tools.

Similar tool to Husky has similar issues: https://github.com/observing/pre-commit/issues

We had same discussions about shell completion and we created the completion generator for it.

What about creating a productivity generator that would group completion, git hooks and other things that require explicit agreement from users?

We could then have a single question like do you want to use productivity add-ons?" and if yes it will call the productivity generator?

Consider also the question about market place, we have now many incompatible modules and some beginners just select a dozen of them and as a result generation fails.

[Pierre BESSON]

Hi Gaël,

I can understand your concerns about precommit hooks. I've also had bad experience with buggy pre-commit hooks at a previous client. However I believe the one lint-staged/prettier/eslint --fix combination is not too intrusive and provide huge value.

You states that it works on windows only for a month but I know it as a fact that this is not true. You also states that it uses NPM so it might conflict with yarn but in my experience it was not the case at all as it uses npm only to start the script and does not need to change the node_modules dependencies.

For my team `lint-staged` has worked very well and was much better than what we did before which was failing the build on lint errors.

I would say let's give it a shot to activate it by default with the 5.0 release but let's add specific instruction in the generated readme to help troubleshoot if people encounter problems (for example, we need to document that it will be active only after npm/yarn install).

Regards,

Pierre

--
You received this message because you are subscribed to the Google Groups "JHipster dev team" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jhipster-dev+unsubscribe@googlegroups.com.
To post to this group, send email to jhipst...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/jhipster-dev/f98e2250-6c7e-475f-abd8-ed1ba42fab80%40googlegroups.com.

[Deepu K Sasidharan]

I'm totally fine with making it optional with a question for the end users and yes we need to do something about the marketplace question. Maybe filter them based on the required-generator version on their package.json

Thanks & Regards,

Deepu

--
You received this message because you are subscribed to the Google Groups "JHipster dev team" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jhipster-dev+unsubscribe@googlegroups.com.
To post to this group, send email to jhipst...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/jhipster-dev/f98e2250-6c7e-475f-abd8-ed1ba42fab80%40googlegroups.com.

[Gaël Marziou]

Anyway, I still can't get the postinstall scripts working on Windows with yarn linked generator, same thing with prettier than with opencollective.

error An unexpected error occurred: "Command failed.                                              

Exit code: 2                                                                                      

Command: C:\\WINDOWS\\system32\\cmd.exe                                                           

Arguments: /d /s /c e:\\projets\\jhipster-projects\\issues\\jhipster-git-precommit-hook\          

ode_modules\\.bin\\prettier --write **/*.ts                                                       

Directory: e:\\projets\\jhipster-projects\\issues\\jhipster-git-precommit-hook                    

Output:                                                                                           

".                                                                                                

[Gaël Marziou]

Hi Pierre,

I can understand your concerns about precommit hooks. I've also had bad experience with buggy pre-commit hooks at a previous client. However I believe the one lint-staged/prettier/eslint --fix combination is not too intrusive and provide huge value.

I'd argue that the value has to be estimated by each user not by our team.

For my projects, I estimate that I can get almost same value without the hassles if those checks are run manually and in continuous integration.

Git hooks are global to your repository, so if you upgrade a project in v5, git hooks will be active even for your maintenance branches in v4 and you might find this annoying because you have to either modify your code, your hooks or use --no-verify for each commit in maintenance.

 

You states that it works on windows only for a month but I know it as a fact that this is not true.

If you're talking about pretiier i don't know, I'm just mentioning the issues in Husky's changelog https://github.com/typicode/husky/blob/dev/CHANGELOG.md

 

You also states that it uses NPM so it might conflict with yarn but in my experience it was not the case at all as it uses npm only to start the script and does not need to change the node_modules dependencies.

Not really what I said but I agree with you, it should work OK.

 

For my team `lint-staged` has worked very well and was much better than what we did before which was failing the build on lint errors.

i don't have same issue as a build failing in CI on a developer branch does not affect the rest of my team.

 

I would say let's give it a shot to activate it by default with the 5.0 release but let's add specific instruction in the generated readme to help troubleshoot if people encounter problems (for example, we need to document that it will be active only after npm/yarn install).

Let's document how to uninstall cleanly husky so that its preinstall script gets executed and let's check that it does not uninstall hooks that it did not install.

 

Regards,

Gael