-
Notifications
You must be signed in to change notification settings - Fork 225
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 eslint, prettier, and re-format JS #213
Conversation
I do not know what is failing here; when I run |
Thanks for your PR. If we can fix the CI I'd be happy to merge it. |
I would if I could, but it seems unrelated. There is no line with |
I will try to find time to look at it, sorry if it takes time |
Looks like this |
@QuLogic sorry for the time it takes to merge your PR. Could you rebase your branch? The CI should pass now. It was stupid conda which was installing a very old nodejs version. |
Rebased. Note, this seems to have bumped the jupyter widgets version in |
Actually, never mind the jupyter widgets version bump. I figured out how to get it to not change that. |
Awesome. Do you think it would make sense to run the lint checker in the CI? So that we make sure we respect the linter rules in the future. I would be in favor of this myself. |
Yes, this is something we did in Matplotlib as well; I can pull that in here, or separately. |
This pulls in the same lint/format method used for ipywidgets, though there are a few differences to minimize changes.
package*.json
which is already 2-space), or else I've have to rewrite basically everything.Some other non-defaullt changes:
if
so they would stay separate lines._
soeslint
wouldn't complain they were unused.Having a common style is helpful for cross-project sharing, and I plan to apply the same change to Matplotlib's nbagg/webagg backends. However, I am not an expert in the JS ecosystem, so I may not have worked around any issues in the best way.
Adding the config and applying the format are separate commits for easier review.