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

Update dependencies and fix ESLint warnings accordingly #2433

Merged
merged 8 commits into from Feb 1, 2018

Conversation

Projects
None yet
2 participants
@igor-savin-ht
Contributor

igor-savin-ht commented Jan 18, 2018

No description provided.

@igor-savin-ht

This comment has been minimized.

Contributor

igor-savin-ht commented Jan 18, 2018

@elhigu Would be great if you could take a look at aaa4d71 -> I've removed it as ESLint demanded me to do so, but I wonder if there was actually a bug there and that param was supposed to be used.

@igor-savin-ht

This comment has been minimized.

Contributor

igor-savin-ht commented Jan 29, 2018

@elhigu I can restore removed code with eslint ignore and add the ToDo to review it later, if it's not an option to check it now :)

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Jan 29, 2018

@igor-savin-ht honestly I just haven't got time to read this through with thought. Smaller changes are easier to accept with confidence that they are not breaking anything :) I would prefer eslint ignore for now.

@igor-savin-ht

This comment has been minimized.

Contributor

igor-savin-ht commented Jan 29, 2018

@elhigu Done!

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Jan 30, 2018

There were few regex where some escape backslashes were dropped. Are you sure they work exactly the same after the changes?

@igor-savin-ht

This comment has been minimized.

Contributor

igor-savin-ht commented Jan 30, 2018

@elhigu ESLint was complaining about those escape backslashes being redundant, I tend to trust its judgement on that. Here is an in-depth discussion on this subject: eslint/eslint#7656
As is with unused variable, I'm open to replacing this change with either line-specific ignore or global rule to ignore useless escapes.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Jan 31, 2018

I'd rather have that global rule set so I need to verify manually e.g. with runkit that each of those regexps are working as expected after change. In this case I don't trust knex testsuite enough and I believe that eslint has them correctly fixed, but I need to be sure.

@@ -153,7 +153,7 @@ function convertTimezone(tz) {
if (tz === 'Z') {
return 0;
}
const m = tz.match(/([\+\-\s])(\d\d):?(\d\d)?/);
const m = tz.match(/([+\-\s])(\d\d):?(\d\d)?/);

This comment has been minimized.

@elhigu

elhigu Feb 1, 2018

Collaborator

Actually I wonder why eslint left \-escaping there.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 1, 2018

@elhigu elhigu merged commit 09eb126 into tgriesser:master Feb 1, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

joelmukuthu added a commit to joelmukuthu/knex that referenced this pull request Feb 16, 2018

Update dependencies and fix ESLint warnings accordingly (tgriesser#2433)
* Update dependencies. Tweak ESLint rules to work more like they used to before

* Fix indentation

* Remove unnecessary escapes.

* Remove unused 'usingClause' parameter.

* Address CI failures.

* Revert "Remove unused 'usingClause' parameter."

This reverts commit aaa4d71.

* eslint: add ignore with todo

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