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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: format code and markup #116

Closed
wants to merge 3 commits into from
Closed

fix: format code and markup #116

wants to merge 3 commits into from

Conversation

sisp
Copy link
Contributor

@sisp sisp commented Nov 4, 2022

Summary

While scanning the code a bit, I noticed it's not consistently formatted using Prettier, although Prettier is used as the code formatter. So I ran Prettier on the entire repo with some ignored files and directories mainly taken from .gitignore.

In some .js/.ts[x] files, the imports weren't sorted properly, so I added the the package prettier-plugin-organize-imports which makes Prettier format imports using the organizeImports feature of the TypeScript language service API. I've used it in other projects in the past and it's a great plugin.

While running Prettier, I saw it choking on

which I've fixed (<bodyp ...> -> <body>) along the way, too.

Sorry for the huge PR, but it's just tons of Prettier formatting changes.

How did you test this change?

There's not much to test, just formatting changes. 馃 that the CI checks pass. 馃槈

@sisp
Copy link
Contributor Author

sisp commented Nov 4, 2022

It seems the build generates a bunch of Markdown files and also some JS/TS code. From what I can tell, Stencil, packages/documentation/scripts/generate-api.mjs, and possibly some other scripts are generating those files. Shouldn't those generated files be ignored by Git? 馃

@danielleroux
Copy link
Collaborator

danielleroux commented Nov 4, 2022

It seems the build generates a bunch of Markdown files and also some JS/TS code. From what I can tell, Stencil, packages/documentation/scripts/generate-api.mjs, and possibly some other scripts are generating those files. Shouldn't those generated files be ignored by Git? 馃

good point. I've also already thought about that.

I think put autogenerated code under version control is not a bad idea at all. (js/ts)

  • You see changes directly in the diff. Some weird autogenerated bugs (not often but happens)
  • If some one checkout the repository and install the node_modules he is ready so browse the code without missing parts (this is also a point for GitHub browser view)

But for the markdown files maybe they can be part of the gitignore file. At least for documentation/docs/Auto-generated because this files are a markdown output of the component-doc.json (packages/core)

@sisp
Copy link
Contributor Author

sisp commented Nov 5, 2022

Alright, I can probably agree on versioning the auto-generated JS/TS code. But then it should be formatted using Prettier before being committed. I doesn't seem that Stencil supports some kind of post-generation command/hook for running Prettier. Perhaps an alternative solution is to use a pre-commit hook, e.g. a setup using Husky + lint-staged might help.

Regarding the auto-generated docs, I've created a PR that removes them and adds the path to .gitignore: #117

@sisp
Copy link
Contributor Author

sisp commented Nov 5, 2022

I guess one problem with versioning generated code is that you might end up with dead files at some point when the code generation changes and you don't remember to manually delete the obsolete files.

@danielleroux
Copy link
Collaborator

Will be intergrated with #197

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.

None yet

3 participants