Apply Linting Only to Files Updated in git

[palisadoes]

We need the ability to selectively adjust our linting rules so that they are eventually applied to our entire code base.

1. Linting rules are managed by the .eslintrc.json file.
2. They are applied to the entire code base at once.
3. If we change a rule from warn to error then every file in the code base will be checked and we have frequently seen our tests fail for dozens of files in hundreds of lines of code.

This is not acceptable.

We have been trying to more strictly lint the entire code base manually, but it has been very slow.

At the user CLI

We need a way to edit .eslintrc.json with increasingly stricter linting rules in such a way that the tests are only applied to the changed files with every git commit and git push command.

These git commands must fail if the linting fails.

With Every Pull Request

We recently merged a Talawa-API pull request that checks each submitted file for linting compliance. This can be used as an example of how to run the test against individual files.

• Update PR_WORKFLOW for delta errors/warning checks talawa-admin#1242

You must make sure that this functionality applied for all PRs for this repository whenever the .eslintrc.json file is updated

Other

1. There may be ways to achieve the CLI goals by updating our existing Git Hooks
2. Your solution should preferably work within coding environments such as VScode
3. Please work with @anwersayeed and @tasneemkoushar on this issue
4. Related issues:
   • Apply Linting Only to Files Updated in git talawa-admin#1270

Screenshots

When complete, you should be able to convert all these .eslintrc.json file lines containing warn to error and the tests will only be applied to the changed files in git

[Gmin2]

can i work on it?

[palisadoes]

@Nitya-Pasrija Please be aware of this issue

[NamitBhutani]

Hi! Can I please work on this issue?

[NamitBhutani]

I have setup and initialised lint-staged for the repo, in order to convert lint rules from "warn" to "error" , should it be done right now or should I wait for approval? @anwersayeed @tasneemkoushar

[NamitBhutani]

@palisadoes I have setup lint-staged (to apply linting to only changed files for every commit). Should I proceed with changing the lint rules from 'warn' to 'error', as I am not sure if this is what was intended to be fixed by this issue.

[palisadoes]

Yes, we want:

1. all the warn rules converted to error
2. linting to run correctly, but only on committed files in the local repository

[palisadoes]

@NamitBhutani
The linting is failing for this PR and probably all new PR updates. Please rectify quickly or we will need to revert your PR:

• Lint only updated files with stricter linting #1635

Context

1. fix: updated setup.ts script to configure and verify SMTP credentials in env file during setup #1647
2. https://github.com/PalisadoesFoundation/talawa-api/actions/runs/7442918033/job/20247042509?pr=1647

[NamitBhutani]

@NamitBhutani
The linting is failing for this PR and probably all new PR updates. Please rectify quickly or we will need to revert your PR:

• Lint only updated files with stricter linting #1635

Context

1. fix: updated setup.ts script to configure and verify SMTP credentials in env file during setup #1647
2. https://github.com/PalisadoesFoundation/talawa-api/actions/runs/7442918033/job/20247042509?pr=1647

Extremely sorry! I'll look into it.
Again, sorry for the trouble!

[NamitBhutani]

@palisadoes The reason the tests are failing for each PR (including this one) is because we have edited the eslintrc.json, thus when npm run lint:check is ran, it uses the updated eslint rc file with errors instead of warns. We will need to update the github actions or make another eslintrc.json file for github workflows.


I have tested the above command, it works!

[palisadoes]

The GitHub action used errors before. Browse the repository a week ago to see the differences in the files you mention then you'll be better able to see what happened

It definitely used errors before, and ran against each file submitted in the PR as part of a for loop

[NamitBhutani]

The GitHub action used errors before. Browse the repository a week ago to see the differences in the files you mention then you'll be better able to see what happened

It definitely used errors before, and ran against each file submitted in the PR as part of a for loop

According to my understanding the job "Check for linting, formatting, and type errors" uses @actions/checkout and then runs the npm run lint:check which is essentially eslint . --max-warnings=1500, i.e it uses the eslintrc.json and as I have edited it to contain errors instead of warnings, that is why the action is erroring.

[palisadoes]

How can we get this to work in the same way as proposed in Talawa Admin where we lint the code with increasing severity until we get it to the desired standard? Here is what they are planning.

• add eslint rules and plugins for "Apply Linting Only to Files Updated in git #1270" talawa-admin#1397 (comment)

[palisadoes]

For example would it be easier to split this up into multiple issues each handling a different linting rule?

[NamitBhutani]

For example would it be easier to split this up into multiple issues each handling a different linting rule?

I feel if we want to keep the PR workflow consistent, we can rather make a separate eslintrc.json for running on staged files (like I mentioned before but not for the PR workflow) with errors and revert the current eslintrc.json to have "warns" instead.

[palisadoes]

1. Unfortunately displaying warnings doesn't lead to action. Errors do.
2. The only way to independently ensure that the linting is done is for the PR workflow to do it in addition to the git workflow on the local branch

Can we do linting for files committed in the local branch with errors, and do the same for the updated files in the PR? If not, then we'll need to create issues per lint rule and close this one.

Can you implement the incremental approach in both scenarios? It hasn't looked promising so far.

[NamitBhutani]

1. Unfortunately displaying warnings doesn't lead to action. Errors do.
2. The only way to independently ensure that the linting is done is for the PR workflow to do it in addition to the git workflow on the local branch

Can we do linting for files committed in the local branch with errors, and do the same for the updated files in the PR? If not, then we'll need to create issues per lint rule and close this one.

Can you implement the incremental approach in both scenarios? It hasn't looked promising so far.

I will try my best to update the PR workflow too , to accommodate for updated files only instead of all the files!

[palisadoes]

@NamitBhutani Your fix is failing it seems.

• https://github.com/PalisadoesFoundation/talawa-api/actions/runs/7772162538/job/21194271619?pr=1787

[palisadoes]

We can't merge Family Members until this is fixed unfortunately

[palisadoes]

Click on the arrow and you see that it failed, but didn't stop the workflow

[palisadoes]

We have a similar PR linting feature in Admin. Check that YAML file for inspiration

[NamitBhutani]

@NamitBhutani Your fix is failing it seems.

* https://github.com/PalisadoesFoundation/talawa-api/actions/runs/7772162538/job/21194271619?pr=1787

I feel that this is something to do with how its fast-forwarding the branch with the develop change we made to fix it, I have one more solution, we can try that too, if you allow.
The solution being using git diff --name-only ${{ github.sha }}^1 instead of the two commit hashes, as the latest hash is not present in the commit history.

[NamitBhutani]

@NamitBhutani Your fix is failing it seems.

* https://github.com/PalisadoesFoundation/talawa-api/actions/runs/7772162538/job/21194271619?pr=1787

I feel that this is something to do with how its fast-forwarding the branch with the develop change we made to fix it, I have one more solution, we can try that too, if you allow. The solution being using git diff --name-only ${{ github.sha }}^1 instead of the two commit hashes, as the latest hash is not present in the commit history.

@EshaanAgg should I make a PR right now if you will be able to merge it?

[lakshz]

@NamitBhutani Just to help you.
In my opinion, you could try just doing npm run lint-staged here, instead of manually checking the changed files.

npm run lint-staged works well in pre-commit husky file, so I think it can easily solve this problem.

[NamitBhutani]

Lint-staged only works in the pre-commit situation, i.e. when the files are staged ( as the name suggests). As here no files are staged, it won't work.

[palisadoes]

Please also verify in Talawa Admin. I'd like to know how to standardize the typescript linting workflows

[palisadoes]

Should we then lint on commit too, to prevent committing from being done?

[lakshz]

Should we then lint on commit too, to prevent committing from being done?

We already have that in pre-commit file.

[palisadoes]

@NamitBhutani Are you sure this is working? We need to merge this PR, that initially discovered the problem

• Added Support for User Family Members #1787

[NamitBhutani]

Please also verify in Talawa Admin. I'd like to know how to standardize the typescript linting workflows

Is it in the main or the develop branch of Talawa admin ?

[palisadoes]

Develop

[NamitBhutani]

Develop

They are using pre-made GitHub actions for checking the changed files, while we are using the git diff command in talawa-api.
But I believe it is run for all typescript files, the one in talawa-api, runs only for changed files.

[palisadoes]

1. Now that we know this method works for PRs we should open an issue to do the same in Admin and run a similar testing regime that we just followed to verify it.
2. Are you willing to do that?

[palisadoes]

Actually, we have something similar created. You may want to provide some guidance here

• feat: lint files which are changed with strict rules talawa-admin#1523
• Apply Linting Only to Files Updated in git #1548

[NamitBhutani]

1. Now that we know this method works for PRs we should open an issue to do the same in Admin and run a similar testing regime that we just followed to verify it.
2. Are you willing to do that?

Yes! Sure

[NamitBhutani]

Actually, we have something similar created. You may want to provide some guidance here

• feat: lint files which are changed with strict rules talawa-admin#1523
• Apply Linting Only to Files Updated in git #1548

Sure, I will look into it.

[devsargam]

@NamitBhutani I am the one who opened the pr
PalisadoesFoundation/talawa-admin#1523

Can you provide some guidance???

[palisadoes]

@NamitBhutani Your git linting seems to be failing again. Pleases take a look.

• Added Support for User Family Members #1787

[NamitBhutani]

@NamitBhutani I am the one who opened the pr PalisadoesFoundation/talawa-admin#1523

Can you provide some guidance???

I feel that using git diff is a bad approach as there are a lot of exceptions that rise up, we'll have to change that from the talawa-api too. We need to use the github action already being used in talawa-admin. You can look into the documentation https://github.com/marketplace/actions/changed-files#outputs- and implement it for talawa-admin.

[NamitBhutani]

@NamitBhutani Your git linting seems to be failing again. Pleases take a look.

• Added Support for User Family Members #1787

I'll soon make another PR to try fixing it.

[devsargam]

@NamitBhutani @palisadoes what about linting on only added files and not modified fields with a seperate eslint config especially for the action???

Example:
If foo.ts was modified then we ignore it
but if bar.ts was added first time in git we lint it with strict mode????

[palisadoes]

Closing. This should be fixed.