Skip to content

Use eslints tests instead of jshint #114

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 20, 2017
Merged

Use eslints tests instead of jshint #114

merged 2 commits into from
Jul 20, 2017

Conversation

Ladsgroup
Copy link
Contributor

the eslint checks still fail:

amir@amir-GL552VW:~/DataValuesJavaScript$ ./node_modules/.bin/eslint . --fix

/home/amir/DataValuesJavaScript/tests/src/dataValues.DataValue.tests.js
  291:6  error  unnecessary '.call()'  no-useless-call

/home/amir/DataValuesJavaScript/tests/src/values/TimeValue.tests.js
  93:6  error  Don't make functions within a loop  no-loop-func

Not sure how to fix those.

thiemowmde
thiemowmde suggested changes Jul 19, 2017
View reviewed changes
Copy link
Contributor

@thiemowmde thiemowmde left a comment

Choose a reason for hiding this comment

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

What do you mean with "fails"? Isn't Travis running this job right now? Why is it green?

.travis.yml
@@ -7,6 +7,9 @@ env:
- RUNJOB=jshint
- RUNJOB=qunit

before_script:
- nvm install 4

script: bash ./build/travis/script.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be replaced with composer test, and the .sh files deleted, just as in all other code repositories we maintain. But this can be done in a later patch.

tests/src/dataValues.DataValue.tests.js Outdated
@@ -124,7 +124,7 @@ define( [
'is instance of actual data value implementation\'s constructor'
);
assert.equal(
typeof( instance.getType() ),
typeof ( instance.getType() ),
Copy link
Contributor

Choose a reason for hiding this comment

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

typeof is not a function. The round brackets should be removed.

Same for all typeof below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked and there was only two, tell me if I'm missing anything.

@Ladsgroup
Copy link
Contributor Author

It's because the checks are not being ran. I fixed that and these will fail right now :)

@Ladsgroup
Copy link
Contributor Author

I said so :D

@thiemowmde
Copy link
Contributor

I updated the this patch a bit. An issue was that the code in the lib/globeCoordinate as well as the tests/lib folders was not tested any more, but should be. I restored this. I also tried to change the code ESLint complains about. I don't know if this is sufficient as I can not run ESLint locally. Please feel free to continue working on this patch if you have an idea.

@thiemowmde thiemowmde force-pushed the eslint branch 2 times, most recently from 3ec798e to eb8020b Compare July 20, 2017 13:33
@thiemowmde
Copy link
Contributor

thiemowmde commented Jul 20, 2017 •
edited
Loading

This succeeds now. I disabled the no-loop-func check and fixed the other violation in the code (it was a test anyway, so this change can not have any impact).

Note that this repository still contains tons and tons of unused code. I want to remove this for years now, and finally found a way to start doing this. Please review #115 if you can. Thanks!

@Ladsgroup
Use space instead of tabs
9dd5f9f 
For consistency
@Ladsgroup
Copy link
Contributor Author

I pushed another commit on top of yours to fix a very nitpicky problem I found. Merging this now.

@Ladsgroup Ladsgroup merged commit 1b6d718 into master Jul 20, 2017
@thiemowmde thiemowmde deleted the eslint branch July 20, 2017 19:33
This was referenced Jul 20, 2017
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thiemowmde thiemowmde thiemowmde approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Ladsgroup @thiemowmde