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

Added eslint,lint-staged and Husky #519

Merged
merged 9 commits into from Jul 14, 2020
Merged

Added eslint,lint-staged and Husky #519

merged 9 commits into from Jul 14, 2020

Conversation

keshav234156
Copy link
Member

Fixes #502

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with grunt jasmine
  • code is in uniquely-named feature branch and has no merge conflicts
  • PR is descriptively titled
  • PR body includes fixes #0000-style reference to original issue #
  • ask @publiclab/reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

We have a loose schedule of reviewing and pulling in changes every Tuesday and Friday, and publishing changes on Fridays.

Thanks!

@keshav234156 keshav234156 changed the title added eslint,lint-staged and Husky [WIP] Added eslint,lint-staged and Husky Jun 1, 2020
@keshav234156
Copy link
Member Author

@VladimirMikulic I believe you once suggested to have a linter for this project, can't recall where? So here it is. There were so many linting errors, still fixing it

@keshav234156 keshav234156 changed the title [WIP] Added eslint,lint-staged and Husky Added eslint,lint-staged and Husky Jun 2, 2020
@keshav234156
Copy link
Member Author

@jywarren
Copy link
Member

jywarren commented Jun 2, 2020

Hi! This looks great. Will it add any extra steps we need to warn other devs of? Things to watch out for? Things we need to mention in the README, maybe? Thank you!!!

@keshav234156
Copy link
Member Author

No, we don't need to give any instruction to future devs. npx eslint command will automatically run before every commit.

@jywarren
Copy link
Member

jywarren commented Jun 3, 2020

Hi, that's great. Just a few more Qs:

  1. is it run in Travis, and can you link me to the lines in the travis output in this PR that show it?
  2. will it block merges if it fails in travis?
  3. if it blocks, how is this shown and explained to people who've opened a PR?

Thank you!!

@keshav234156
Copy link
Member Author

Nope, when a person commit the the changes with git commit command then npx eslint --fix will run and it will fix all the linting errors

@emilyashley
Copy link
Member

emilyashley commented Jun 3, 2020

Cool, thanks Keshav. I'm a fan of linting -- especially if it's standard across dev environs and repos.

Just curious if this plays nice with Git GUIs (i.e. Github Desktop) or if it requires developers to be using the git CLI. (sorry if this question was already addressed and I missed it)

@keshav234156
Copy link
Member Author

@emilyashley it works only with git CLI. I believe mostly use the git CLI for their work. If someone uses the Git GUI then this won't work but whenever next developer who uses the git CLI makes a commit then it would fix linting issues of previous developer as well as we are running eslint through all js files in the repository

@emilyashley
Copy link
Member

emilyashley commented Jun 3, 2020

Cool, just noting that the more linting differs between developer environments the more linting changes can make Pull Requests and diffs harder to read.

I and plenty of other developers I know use GUIs like Github Desktop or Source Tree to help manage version control and collaboration.

But if the community is down with it I am. 👍 cc: @Shulammite-Aso @Shreyaa-s @NitinBhasneria

@govindgoel
Copy link
Member

govindgoel commented Jun 10, 2020

@keshav234156 Can you resolve the conflicting files and also squash the commits.

@jywarren
Copy link
Member

Hi @keshav234156 - thank you. Just to clarify, will this then be a blocker for folks using non-CLI systems? If not, I think we are OK with it, but we should note that it's optional.

Apart from that i agree with @emilyashley that including linting changes in regular PRs is not ideal for readability reasons. Perhaps this means that we should also open some PRs to lint the entire codebase as a project unto itself, so that PRs don't include linting of entire files every time they're touched? But, open to ideas from either of you! Thanks @govindgoel too!

@keshav234156
Copy link
Member Author

@jywarren This will not at all be a blocker for non-CLI systems. Though I believe readibility become better if we have code is proper linted and have a proper indentation. Also, we have already been using it in Image-Sequencer and it works pretty well.
Thanks!!

anshukaira and others added 3 commits June 17, 2020 19:04
Co-authored-by: Cess <cessmbuguar@gmail.com>
adds .vscode in .gitignore
adds _SpecRunner.html in .gitignore

Co-authored-by: Emily Ashley <15912063+emilyashley@users.noreply.github.com>
Co-authored-by: Govind Goel <52847415+govindgoel@users.noreply.github.com>
Co-authored-by: Cess <cessmbuguar@gmail.com>
@jywarren jywarren changed the base branch from master to main June 24, 2020 15:32
@cesswairimu
Copy link
Collaborator

Hi @keshav234156 could you please fix the conflicts ...thanks

@keshav234156
Copy link
Member Author

@jywarren @Shulammite-Aso @Shreyaa-s @NitinBhasneria Can you please help me here? Test are failing after I fixed merge-conflict. It shows

@jywarren
Copy link
Member

Hi! I'm sorry, there are still some merge conflicts to resolve. Apologies on this as we're getting the next release set up, but we should be able to merge this and move forward if you can resolve these. Thank you!

@keshav234156
Copy link
Member Author

@jywarren Doing this right now

@gitpod-io
Copy link

gitpod-io bot commented Jul 14, 2020

@keshav234156
Copy link
Member Author

@jywarren Done!!

@jywarren
Copy link
Member

Great work here. Merged!

@cesswairimu
Copy link
Collaborator

🎉 🎉

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

Successfully merging this pull request may close these issues.

Adding a linting tool
8 participants