Skip to content

Conversation

@Indigo744
Copy link
Collaborator

I suggest we enforce more JSHint rules to bring Mapael up to current JS standard.
Also, it helps us catch more bug or typo (see the one at line 1470 that was not detected before).

The following JSHint rules are added:

    undef: true prohibits the use of explicitly undeclared variables
    eqeqeq: true enforce strict equality (=== and !==)
    forin: true requires all for in loops to filter object's items
    freeze : true prohibits overwriting prototypes of native objects
    nocomma : true prohibits the use of the comma operator
    nonew : true prohibits the use of constructor functions for side-effects

The following options are also added, only to remove some wrong warnings:

    esversion: 3 Legacy ECMAScript version (IE9 and below)
    browser: true use browser env
    devel: true allow console

The following JSHint rules are deleted (no longer relevant):

    laxcomma: true allow comma first coding style
    laxbreak: true allow unsafe line breaking

Also, all comments are removed from .jshintrc file as they are forbidden by JSON standard.
Instead, justification is in a Wiki page: https://github.com/neveldo/jQuery-Mapael/wiki/JSHint

@Indigo744 Indigo744 requested a review from neveldo October 5, 2017 14:43
Copy link
Owner

@neveldo neveldo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Indigo744 ,

Thanks for your PR. I think it will lead to a great improvement of the overall quality of the code.

I have just noticed what seems to be a little mistake in the CONTRIBUTING.md (I hope your adds to this file will motivate some more users to contribute !).

When fixed, everything should be ok to be merged !

CONTRIBUTING.md Outdated
## Test suite

There is a test suite in the `/test` folder, power by [QUnit](http://qunitjs.com/).
While not broken, it is however, it is very light. We sadly don't have the time to expand it for now.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a small mistake here ?

it is however, it is very light. => it is however very light.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

@Indigo744 Indigo744 merged commit 07758b7 into neveldo:master Oct 11, 2017
@Indigo744 Indigo744 deleted the better_jshint branch October 11, 2017 08:52
@Indigo744
Copy link
Collaborator Author

I've squashed and merged this PR (using option provided by GH), so there's not too much commit! I think we should try to do this as much as possible to reduce cluttering the commit tree :)

@neveldo
Copy link
Owner

neveldo commented Oct 11, 2017

Thanks for the fix and for the merge ! You juste made me discover this shortcut on github, I didn't know about it before ... I agree with you, it will allow the project history to be more clear and simple, I will try to think about it for the future merges !

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.

2 participants