search

TEAMMATES / teammates

This is the project website for the TEAMMATES feedback management tool for education
https://teammatesv4.appspot.com/
GNU General Public License v2.0
1.66k stars 3.31k forks source link

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

Closed fsgmhoward closed 1 year ago

fsgmhoward commented 2 years ago

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.

FergusMok commented 2 years ago

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 commented 2 years ago

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

fsgmhoward commented 2 years ago

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

tenebrius1 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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