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

Linting should be skipped for generated files during local build and test #11564

Closed
fsgmhoward opened this issue Feb 11, 2022 · 6 comments · Fixed by #11910
Closed

Linting should be skipped for generated files during local build and test #11564

fsgmhoward opened this issue Feb 11, 2022 · 6 comments · Fixed by #11910
Labels
a-Build CI, task automation, build pipeline help wanted Moderate difficulty, small localized change; suitable for novice contributors

Comments

@fsgmhoward
Copy link
Member

This is a very minor improvement which would improve local testing efficiency in my opinion.

Steps to reproduce

Run npm lint (specifically, it affects lint:css and lint:space).

Basically, the auto generated files of npm run build has been linted, resulting a large number of errors thrown out. Also, json files generated by lnp tests also fails the test.

Expected behaviour

Only linting whatever to be pushed to the git repository, skipping all the auto-generated items (such as those listed in .gitignore).

Additional info

For the lint:css, issue is resolved by adding --ignore-pattern \"src/web/dist/*.css\" behind the current command line strings. This instructs the stylelint not to check files in dist folder (the generated ones for production).

For the lint:space, lintspaces program does not have a good way to skip certain files. Probably a better matching rules needed to match files for linting.

@fsgmhoward fsgmhoward changed the title Generated files during component test and linting should be skipped Linting should be skipped for generated files during local build and test Feb 11, 2022
@wkurniawan07 wkurniawan07 added a-Build CI, task automation, build pipeline help wanted Moderate difficulty, small localized change; suitable for novice contributors labels Feb 18, 2022
@FergusMok
Copy link
Contributor

This may be somewhat related, but the better solution may be to just lint files that have diff. Currently, npm run lint lints all typescript/html files, which is unnecessarily long. This method will solve this issue and also improve the efficiency of linting.

@tenebrius1
Copy link
Contributor

Hello, is anybody investigating this issue? If not I don't mind looking into it!

@fsgmhoward
Copy link
Member Author

@tenebrius1 I don't think anyone is doing that. Please proceed.

@tenebrius1
Copy link
Contributor

@fsgmhoward I have a few questions regarding this issue:

  1. I'm not too sure what the team's stance is with regards to solutions that add in new dependencies so I just wanted to check if it is okay if a solution I found requires adding in a new NPM package -- globby as suggested in this issue of the lintspaces-cli repo. I have tested this package and it allows for negated patterns e.g. "!src/web/dist/*" which would effectively exclude all the build files in the dist folder.
  2. Are there any specific files/path that you are looking to exclude for the lintspaces program? So far I can only identify the files in src/web/dist as build files to be excluded. Could you point me to any documentation that mentions where the build files are being generated?

@fsgmhoward
Copy link
Member Author

@tenebrius1

  1. As far as I know we are quite loose about npm dependencies since they are used frontend and do not impose much security issue to the production data. You may proceed if you think it is necessary
  2. I do not have a list of them. However, as @FergusMok suggested above, you may actually consider making it to lint files with git diff only. This will effectively skips files unchanged and those should be excluded (e.g. those in .gitignore).

@tenebrius1
Copy link
Contributor

@fsgmhoward Thanks for the clarification! I will look into whether there is a way to only run linting on files with git diff.

zhaojj2209 added a commit that referenced this issue Oct 25, 2022
…ildand test (#11910)

* Fix lint:css issue

* Update lint:spaces to ignore build files

* Update linting to use git diff

* update linting routine to be ran as pre-commit hook

* improve linting on github actions

* modify git diff command to work on github actions

* add fetch-depth flag to actions/checkout

* remove trailing space

* remove pre-commit hook

Co-authored-by: Nicolas Chang Weng Yew <[email protected]>
Co-authored-by: Zhao Jingjing <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-Build CI, task automation, build pipeline help wanted Moderate difficulty, small localized change; suitable for novice contributors
Projects
None yet
4 participants