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

Issue #72 -- Add a Linter #83

Closed
wants to merge 23 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@reficul31
Copy link
Contributor

reficul31 commented Apr 3, 2017

Features of the PR.

  • Adds eslint to the project for consistent styling
  • Adds editor config to the project
  • Configures to remove most style errors and warnings.

@Treora Please take into account that the PR still has some errors. Please either remove the errors or remove the lint itself. Please check and see if the rules are to the maintainers liking, or they can always be changed. Thanks.

USAGE

  • sudo npm run lint -- for checking the errors in the lint
  • sudo npm run lint-fix for automatically fixing the errors
# Styling applicable to the file extension given
[*.js, *.jsx]
# This is due to the fact that github allows only 119 columns of space to view
max_line_length = 119

This comment has been minimized.

@obsidianart

obsidianart Apr 3, 2017

Collaborator

I would really discuss this one

This comment has been minimized.

@reficul31

reficul31 Apr 3, 2017

Author Contributor

Actually this is a widely observed standard in many open source projects

max_line_length = 119

This is done so that there is no need for horizontal scrolling while reading the code as github provides only 119 columns worth of space to read the code. But I can change if it seems too restricting.

This comment has been minimized.

@obsidianart

obsidianart Apr 3, 2017

Collaborator

@Treora makes the final decision, I wouldn't have this rule

This comment has been minimized.

@bwbroersma

bwbroersma Apr 3, 2017

The reason here is actually 120, but minus 1 char for the diff view. Why 120 I don't get because it seems pretty much browser specific, and people just choose a round number, see:
screenshot of github editor in different browsers on OsX
(source Django developers discussion)
For more random numbers, see this stackoverflow thread.

I do agree setting line lengths to something higher than 80, but 119 and binding that to Github web view and some browser specifics and then rounding that number seems pretty ... arbitrary.

This comment has been minimized.

@obsidianart

obsidianart Apr 3, 2017

Collaborator

I understand github diff works on this length, I'm not discussing the number, I'm dicussing having any number there.
If tomorrow (real life story) I need to put an svg encoded icon somewhere I do want it on one line and I don't care if you have to scroll.
I'm not really worried of a dev writing a very very long line, why would it do that? So I consider this rule more a possible issue than a safeguard.

This comment has been minimized.

@reficul31

reficul31 Apr 3, 2017

Author Contributor

Point noted, I shall remove this line immediately. Or would you like another value to takes its place?

This comment has been minimized.

@Treora

Treora Apr 4, 2017

Collaborator

I intended to ask about it too; adjusting rules to Github's presentation seems somewhat arbitrary (or submissive ;) ) indeed.

I have myself tried to keep things at a soft limit of 80 characters, though I'd be fine with a larger number too. As said it is nice to be able to make longer lines if needed; I am not sure whether editors interpret such a rule as soft or hard.

This comment has been minimized.

@obsidianart

obsidianart Apr 4, 2017

Collaborator

If we have a linter it will probably stops you from committing if you have linter errors, so I would say it will be pretty hard :)

@reficul31 reficul31 force-pushed the reficul31:issue_72 branch from 5c6b412 to ba2860f Apr 3, 2017

@reficul31

This comment has been minimized.

Copy link
Contributor Author

reficul31 commented Apr 3, 2017

There are 10(5 warnings and 5 errors) persisting errors that I cannot resolve as they require some big design decisions. It would be really helpful if someone could help me with them.

no-unused-vars error

This error finds its source. It is currently set to warning. Places of occurrence

src/activity-logger/background/log-page-visit.js => (finalPagePromise , visit) both are unused vars

react/no-unused-prop-types error

This error finds it source. It is currently set to warning. Places of occurrence.

src/options/layout.jsx => (location, children) both are unused prop types

prefer-promise-reject-errors error

This error finds its source. It is currently set to error. Places of occurrence

src/util/event-to-promise.js => line 58

promise/catch-or-return error

This error finds its source. It is currently set to error. Places of occurrence

src/util/tab-events.js => line 29

handle-callback-err error

This error finds its source. It is currently set to error. Places of occurrence

src/util/webextensionRPC.js => line 27 and 17
src/util/when-all-settled.js => line 3

These are all the errors that are still there. If we could remove all the errors or remove their lint rules then the code will be fully clean. I will continue looking for more rules that can help make the project more consistent. Thanks. Once the rules are finalized and integrated I will continue to add the git hook.

@reficul31 reficul31 referenced this pull request Apr 3, 2017

Closed

Add a linter #72

4 of 5 tasks complete
@Treora

This comment has been minimized.

Copy link
Collaborator

Treora commented Apr 4, 2017

Thanks for the PR. I will do another round of changes to remove (or suppress) the remaining warnings/errors.

Treora added a commit that referenced this pull request Apr 5, 2017

@Treora

This comment has been minimized.

Copy link
Collaborator

Treora commented Apr 5, 2017

Done & merged. GitHub seems not to have detected that, so closing this manually. Thanks for the effort!

@Treora Treora closed this Apr 5, 2017

@reficul31

This comment has been minimized.

Copy link
Contributor Author

reficul31 commented Apr 5, 2017

@Treora thanks for all the help!!

reficul31 pushed a commit to reficul31/webmemex-extension that referenced this pull request Dec 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment