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

Fix JSHint Warnings / Errors and remove two "bad" test cases in FetchSpec. #189

Closed
wants to merge 1 commit into from

Conversation

StrongRef
Copy link
Contributor

No description provided.

@LeaVerou
Copy link
Owner

LeaVerou commented Jun 20, 2016

Hi there,

Thanks for contributing!
In the future, please try to separate different things into different PRs.

I do not agree with the code style changes. Point taken though, I should replace .hshintrc with an .eslintrc.json, I haven't used JSHint in a while which explains why its config is out of date.

Why remove the testcases? What's wrong with them?

@StrongRef
Copy link
Contributor Author

StrongRef commented Jun 20, 2016

Those tests are failing when using Sinon 1.17.4.

I agree that this could be multiple PRs but they do fix warnings / errors for gulp and npm test, that is why I went with a single PR.

We could replace Sinon ~1.17.2 with 1.17.3 and replace JSHint with ESLint.
Should I close this?

Update:
I checked the failing-tests again and I think this is a Sinon bug...
For some reason onerror / onloaded is not called when stubbing a 404-response.
Probably: sinonjs/sinon@2cfbacd

@LeaVerou
Copy link
Owner

Yes. I will add the eslint config file and update the sinon version asap.

@StrongRef StrongRef closed this Jun 20, 2016
@LeaVerou
Copy link
Owner

Done

@StrongRef
Copy link
Contributor Author

StrongRef commented Jun 20, 2016

Thanks, still some issues though.

  • "sinon": "^1.17.3" => "sinon": "1.17.3" version 1.17.4 is broken.
  • Remove JSHint preprocessor from karma.conf.js. (jshint and karma-jshint could probably be removed from package.json as well)

We should probably use ESLint from karma? Let me know if you want me to look into this one.

@LeaVerou
Copy link
Owner

We should probably use ESLint from karma? Let me know if you want me to look into this one.

Yes! If you know how to do it, any PRs would be appreciated! Otherwise I can look into it!

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