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 Prettier to format YAML #9281

Merged
merged 1 commit into from Feb 26, 2019

Conversation

Fydon
Copy link
Contributor

@Fydon Fydon commented Feb 17, 2019

Added formatting of YAML files to Prettier, formatted comments in YAML files so that they end up in the correct indentation and changed YAML files that are formatted with an indent of 2 space to 4

Fix #8805

  • 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

@Fydon Fydon force-pushed the feature/UsePrettierToFormatYaml branch from 224a087 to be28596 Compare February 17, 2019 15:45
@DanielFran
Copy link
Member

DanielFran commented Feb 17, 2019

@Fydon can you also include yaml files to format for example Helm files available in Kubernetes folder? Thanks

Edit: forget, yaml files were already updated to yml.

@murdos
Copy link
Contributor

murdos commented Feb 17, 2019

Changes in packages.json.ejs is fine. But the change of formatting in templates is not required.
Instead, you should add the yml extension to this line: https://github.com/jhipster/generator-jhipster/blob/master/generators/generator-base-private.js#L1304 so YAML files will be prettified during the project generation.

It's way better to let Prettier fix our templates on the fly than to maintain prettified templates ;)

@murdos murdos self-requested a review February 17, 2019 21:17
@Fydon
Copy link
Contributor Author

Fydon commented Feb 17, 2019

@DanielFran my current pull request focuses on the current definition of applying Prettier to the src folder. In my project I've extented formatting to the entire project, but with the Prettier ignore file slightly tweaked.

@murdos Some of the formatting is for consistency, so technically unnecessary, but other formatting is due to Prettier formatting comments in such a way that puts them at the wrong indentation so that the YAML is now incorrect, potentially leaving a new user lost when their project is broken when they uncomment. Therefore I felt it worth testing the output and tweaking the comments to be positioned where we would expect them to be, rather then where Prettier has put them. This is why leaving Prettier as a post process could cause problems. Also converting files from 2 space indentation to 4 to match the Prettier configuration, might help having future comments indent to the correct level.

@Fydon Fydon force-pushed the feature/UsePrettierToFormatYaml branch 2 times, most recently from f061aa7 to 64f2e25 Compare February 18, 2019 08:23
@Fydon
Copy link
Contributor Author

Fydon commented Feb 18, 2019

Is the generator using the Prettier from the package.json at the root? The React and Angular package.json files are now using the latest versions, so I don't know why the builds started failing with the error No parser could be inferred for file: src/main/docker/app.yml when I added this change to have Prettier run after generation.

@murdos
Copy link
Contributor

murdos commented Feb 18, 2019

Is the generator using the Prettier from the package.json at the root?

Yes. So I guess we should update to the latest version here too.
Could you do that? Thanks.

@Fydon
Copy link
Contributor Author

Fydon commented Feb 18, 2019

Yes I'll update that.

Should I update eslint-config-prettier and eslint-plugin-prettier as well? Would that mean that I should also update eslint and the other related dependencies?

As a result of updating Prettier for React, I also updated tslint and tslint-config-prettier, which makes them the same as Angular.

@murdos
Copy link
Contributor

murdos commented Feb 18, 2019

Should I update eslint-config-prettier and eslint-plugin-prettier as well? Would that mean that I should also update eslint and the other related dependencies?

Yeah, please update them too.

@Fydon Fydon force-pushed the feature/UsePrettierToFormatYaml branch from 64f2e25 to 09a4cdd Compare February 18, 2019 13:11
@Fydon
Copy link
Contributor Author

Fydon commented Feb 18, 2019

Sure. Updated them. I've looked through the change logs, but didn't see any problems: eslint, eslint-config-prettier, eslint-plugin-import and eslint-plugin-prettier.

@Fydon Fydon force-pushed the feature/UsePrettierToFormatYaml branch from 09a4cdd to 388e070 Compare February 18, 2019 13:19
@murdos murdos closed this Feb 19, 2019
@murdos murdos reopened this Feb 19, 2019
@DanielFran DanielFran closed this Feb 20, 2019
@DanielFran DanielFran reopened this Feb 20, 2019
@Fydon
Copy link
Contributor Author

Fydon commented Feb 21, 2019

What is the reason for setting eslint-plugin-prettier to error on files not conforming to the Prettier formatting? I thought the Prettier formatting would be applied as a post process to "fix" any files. I assume that this is resulting in this test failing.

@Fydon Fydon force-pushed the feature/UsePrettierToFormatYaml branch from 388e070 to 6b0a751 Compare February 25, 2019 08:26
@Fydon
Copy link
Contributor Author

Fydon commented Feb 25, 2019

  1. Following on from my previous comment, should we use something like lint-staged to force running eslint --fix or Prettier on commit?
  2. Should I include .yml in package.json so that .travis.yml and azure-pipeliness.yml are formatted by Prettier?

I've now run eslint --fix so that the tests should pass.

@murdos
Copy link
Contributor

murdos commented Feb 25, 2019

1. Following on from my previous comment, should we use something like `lint-staged` to force running `eslint --fix` or Prettier on commit?

We don't do that for other files, so no.

2. Should I include `.yml` in [package.json](https://github.com/jhipster/generator-jhipster/pull/9281/files#diff-b9cfc7f2cdf78a7f4b91a753d10865a2R39) so that `.travis.yml` and `azure-pipeliness.yml` are formatted by Prettier?

I'm not sure.
@pascalgrimaud : what do you think?

I've now run eslint --fix so that the tests should pass.

Great! Could you also resolve the conflicts? Then we should be able to merge.

@pascalgrimaud
Copy link
Member

I'm ok for formatting the Travis and Azure file, used for our Continuous Integration, if it doesn't break anything :-)

@Fydon Fydon force-pushed the feature/UsePrettierToFormatYaml branch from 6b0a751 to 34c56fe Compare February 26, 2019 08:07
@Fydon
Copy link
Contributor Author

Fydon commented Feb 26, 2019

I've now also applied the formatting to the Travis and Azure files.

@murdos I only suggested lint-staged because that is what the Angular and React projects generated by this project use, but sure I'll leave it as is.

I've resolved the conflicts, which included package-lock.json. I can't tell for certain what the result of dependencies are for package-lock.json, so I instead used npm to provide a valid package-lock.json with the resolved package.json file. I'll wait for the tests to complete, but probably should rather merge current changes into my branch and then push my branch again to minimise the differences.

Added formatting of YAML files to Prettier, formatted comments in YAML files so that they end up in the correct indentation and changed YAML files that are formatted with an indent of 2 space to 4. Updated Prettier, eslint and related dependencies.

Fix jhipster#8805
@Fydon Fydon force-pushed the feature/UsePrettierToFormatYaml branch from c3f7525 to 634f511 Compare February 26, 2019 09:33
@Fydon
Copy link
Contributor Author

Fydon commented Feb 26, 2019

I've rebased my changes to the latest master even though all the tests passed, as it probably results in a better package-lock.json.

@murdos murdos merged commit 7edc07b into jhipster:master Feb 26, 2019
@murdos
Copy link
Contributor

murdos commented Feb 26, 2019

Thanks Fydon ! 👍

@Fydon Fydon deleted the feature/UsePrettierToFormatYaml branch February 26, 2019 13:05
@jdubois jdubois added this to the 6.0.0-beta.0 milestone Apr 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Prettier for yaml
5 participants