Fixes #89 - Add prettier support #105
Conversation
|
Thanks for submitting. If you prefix the issue number with Fixes it will automatically link the issue. |
swarna-gopalan
changed the title
There was a problem hiding this comment.
Thanks for submitting this!
When I tried testing this branch, and did a test commit, it looked like it ran a hook but the commit still appeared unformatted.
Can you add a command in package.json under scripts that lets a developer run the same command as the hook to test it out?
package.json
Outdated
| "eslint-config-airbnb": "^18.1.0", | ||
| "eslint-config-prettier": "^6.10.0", | ||
| "eslint-plugin-import": "^2.20.2", | ||
| "eslint-plugin-jsx-a11y": "^6.2.3", |
There was a problem hiding this comment.
Can you remove the eslint-plugin-jsx-a11y package and make sure it's removed from package lock as well? JSX is only needed for React and we aren't currently using React.
There was a problem hiding this comment.
@vegetabill - "Can you add a command in package.json under scripts that lets a developer run the same command as the hook to test it out" - Not sure I follow this part.
There was a problem hiding this comment.
I'm not sure how to debug the hook but we can start with having an npm script that can lint all files. In package.json we can add something like:
"scripts": {
...
"lint": "npx eslint ./JS/**/*.js ./public/scripts/**/*.js"
},So we can run the linting separate from commit hook. That might help debug the issue of why it doesn't seem to be working. When I added that, the linting wasn't working so that may be why the hook isn't working either.
There was a problem hiding this comment.
How about just "lint": "eslint ./;" ? This is what I see in our repo and we use eslint.
There was a problem hiding this comment.
Also @vegetabill - I'm running into more issues when I run "npm run lint"
Error: Failed to load parser 'babel-eslint' declared in '.eslintrc': Cannot find module 'babel-eslint'
But when I remove it, it says,
ESLint couldn't find the plugin "eslint-plugin-jsx-a11y".
So after adding that using npm install eslint-plugin-jsx-a11y@latest --save-dev, I get 401 problems (390 errors, 11 warnings) when I run linting. I think its looking at rules under /Users/swarna.gopalan/Hackathon/ghc/techtonica/keyboard-shortcuts-practice/node_modules/eslint-config-airbnb.
|
hi @swarna-gopalan , are you still out there? Will you be able to make changes today? |
|
Hi Alina,
I'll give it a try or I can still work on this tomorrow if that's ok?
Thanks!
On Thu, Oct 1, 2020 at 7:12 PM Alina L. ***@***.***> wrote:
hi @swarna-gopalan <https://github.com/swarna-gopalan> , are you still
out there? Will you be able to make changes today?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#105 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABHUP326BGR6RO5YFXAA67TSIULGRANCNFSM4SA3W5TQ>
.
--
Swarna
|
|
As a simplification, we can drop all the linting aspects and just setup prettier config and commit hook. That would mostly cover what is important. @alodahl what do you think? In retrospect, I probably picked an overly complicated blog 😕 |
|
@vegetabill i guess, but what would that be eliminating? Maybe a different tutorial is necessary; I didn’t think it would be very complex, just the right configurations. |
|
@alodahl it would just be adding a set prettier config so that anyone using something like vscode's formatting would all be using the same rules. Since the code is quite non-standard, eslint might be a tough transition at this point. Either way, since this was mainly to make OSD easier (oops), I'd say let's just close this PR unmerged and just throw the issue back into the to-do list for someone who knows of a better setup. Sorry! |
fixes #89 Add prettier support and enforce with commit hook