-
Notifications
You must be signed in to change notification settings - Fork 3
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
Update linters script #79
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Rejection
Dear Diego,
Thank you for your contribution to the project! I appreciate the time and effort you've put into this pull request. However, after careful review, I've decided not to merge it at this time.
Reasons for Rejection
- The command
npm run lint
is working well, but I've found a little issue:
If there are errors in both JS and CSS files, that command shows only JS errors, so the tester or user needs to fix all the JS errors and re-run thenpm run lint
command to see the CSS errors. This behavior can introduce misunderstandings because the user can think that all the JS & CSS linters errors are solved when there are still some CSS errors.
Before introducing JSX linters errors:
as you can see, the CSS errors are being shown
After introducing JSX linters errors:
as you can see, the CSS errors are NOT being shown
Next Steps
I encourage you to address the feedback provided and resubmit the pull request. Feel free to reach out if you have any questions or need further clarification.
Optional Suggestions
- Review this warning
29:6 warning React Hook useEffect has missing dependencies: 'API_URL_BASE' and 'token'. Either include them or remove the dependency array react-hooks/exhaustive-deps
at this component/home/dani/WD/Book-a-concert-front-end/src/pages/DeleteConcertPage.jsx
when running this commandnpm run lint
Thank you again for your contribution and understanding.
Best regards,
Dani
@danifromecuador Thanks so much for noting that out, I think that yeah, it's important to see at once all the linting errors to get a better perspective about what we are doing wrong, so I 'll correct, thanks! and regarding to your optional suggestion is something I'm working on in a different branch, thanks! |
… on the console if any liting error
@danifromecuador After updating the script, now it will show both ESlint and Stylint results, I have added errors only in my local repo to generate some failings in linting tools to verify this behavior: If there is something else let me know! thanks @danifromecuador |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Congratulations
Dear Diego, your PR is approved, you made a great effort and now both ESLint and Stylelint errors are being shown.
There is nothing else to say than:
Implemented Changes:
we have updated the script
lint
inpackage.json
in order to run all installed linting tools on this project witha single command, son when you type
npm run lint
in your console the next linting tools will run:This will avoid the developer having to run
npx eslint "**/*.{js,jsx}"
andnpx stylelint "**/*.{css,scss}"
separately inthe console, but run them together instead with a single command
npm run lint
which is easier to remember.