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

Add lint pre-commit hook #66

Merged
merged 2 commits into from
Jan 28, 2022
Merged

Add lint pre-commit hook #66

merged 2 commits into from
Jan 28, 2022

Conversation

ruleeeer
Copy link
Contributor

No description provided.

@wu-sheng wu-sheng requested a review from kezhenxu94 January 28, 2022 03:16
@apache apache deleted a comment from tkNobug Jan 28, 2022
@kezhenxu94 kezhenxu94 added this to the 0.4.0 milestone Jan 28, 2022
@kezhenxu94
Copy link
Member

Thank you @ruleeeer , can you please fix the CI

@wu-sheng
Copy link
Member

Is the huge package-lock.json change expected? 9998 lines added. It seems crazy.

@ruleeeer
Copy link
Contributor Author

@kezhenxu94
I commited a PR here,But there are still three possible problems,Do you need me to handle this?

  1. When I install using npm install(before I add the dependencies ), it shows

image

  1. has been deprecated,Do you need to switch to eslint

image

  1. The current tslint configuration header is not valid, because no match field check is configured

image

@ruleeeer
Copy link
Contributor Author

Thank you @ruleeeer , can you please fix the CI

I'll solve the CI failure problem

@ruleeeer
Copy link
Contributor Author

image

Is it meaningful to add a license to this file? Because this is generated by Husky

@kezhenxu94
Copy link
Member

@kezhenxu94

I commited a PR here,But there are still three possible problems,Do you need me to handle this?

  1. When I install using npm install(before I add the dependencies ), it shows
image

It's nice if you'd like to upgrade the 2 modules, but please do it in another PR, it's also fine if you don't want.

  1. has been deprecated,Do you need to switch to eslint
image

Yes. I know tslint was deprecated. I appreciate if you can try to replace with eslint. 😃

  1. The current tslint configuration header is not valid, because no match field check is configured
image

The configuration worked for me in my environment, if you switch to eslint you can migrate the configurations also, I can test in my environment

@kezhenxu94
Copy link
Member

kezhenxu94 commented Jan 28, 2022

image

Is it meaningful to add a license to this file? Because this is generated by Husky

You can add an ignore rule to ignore this file (in .licenserc.yaml ) if you don't think it's necessary to add header for it. I'm ok for both

@ruleeeer
Copy link
Contributor Author

Is the huge package-lock.json change expected? 9998 lines added. It seems crazy.

I try to run 'npm install' using the master branch (my changes are not included here), and then use 'git diff -- stat' to display 8954 new lines
image

It should be caused by the upgrade of the lock file due to the different version of npm,I used [email protected] and [email protected]

@kezhenxu94
Copy link
Member

@ruleeeer looks like it's because of the lockfileVersion, I'm using node==14.15.1 and npm==6.14.0, its lockfileVersion is 1 but yours is 2, is it possible for you to install node 14 and re-submit the changes? We can consider upgrade the development version of nodejs but currently let's focus on the lint in this PR

@ruleeeer
Copy link
Contributor Author

@kezhenxu94 sure,I'll push it again with node@14

@ruleeeer
Copy link
Contributor Author

@kezhenxu94 i pushed with node@14,Looks like fewer changes in lockfile

@kezhenxu94 kezhenxu94 changed the title add lint pre-commit hook Add lint pre-commit hook Jan 28, 2022
@kezhenxu94
Copy link
Member

Hi @ruleeeer , I test in my machine and the hook works great, but the rules are not working as expected (did not pick up the config like tslint tslint -p src/tsconfig.json src/**/*.ts), will you be able to adjust the rules any sooner, if not, the hook will prevent any changes if I touch any file, you can try to modify src/Log.ts and it will always fail the lint.

@ruleeeer
Copy link
Contributor Author

@kezhenxu94 This is the third issue that I mentioned before.match field are required and the other fields are optional,
image

You can see the relevant documentation here

you can just add the match field(my '123' is just for verification, it doesn't make any sense)
image


after executing `npm run lint` to get this error message

image

@kezhenxu94
Copy link
Member

Hey @ruleeeer , in my side I can run npm run lint successfully without any changes (is it because of different versions too?). What I meant is npm run lint-staged, it always fail.

Anyway, I'm big +1 to migrate from tslint to eslint (and the rules should be rewrite, then this problem is gone, right?), but I would expect it done soon otherwise the hook will always prevent the changes because the lint always fail. I can merge this first if you would start to migrate the linter

@ruleeeer
Copy link
Contributor Author

@kezhenxu94 Strange, I also found the problem you mentioned, but I tried several methods and couldn't solve it. I'm going to start migrating to eslint.

@kezhenxu94
Copy link
Member

@kezhenxu94 Strange, I also found the problem you mentioned, but I tried several methods and couldn't solve it. I'm going to start migrating to eslint.

Go ahead please. Let me merge this for now and wait for your next PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants