Skip to content

WIP: changes to standardjs code style #324

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

Closed
wants to merge 1 commit into from

Conversation

gabeio
Copy link
Member

@gabeio gabeio commented Jun 15, 2016

There are still a few(actually a lot of the same) errors on the standard that I'm not understanding how to fix (90% are the test/session.js).

here's just a few:

  • session/index.js:87:7: 'options' is already defined
  • session/index.js:162:59: Unexpected use of comma operator.
  • session/session/store.js:30:1: The '__proto__' property is deprecated.
  • session/test/session.js:17:1: 'describe' is not defined.
  • session/test/session.js:18:3: 'it' is not defined.
  • session/test/session.js:27:20: Expected consistent spacing
  • session/index.js:447: Unexpected mix of '||' and '&&' no-mixed-operators
  • session/test/session.js:2350: Unnecessary escape character: \. no-useless-escape

@gabeio gabeio force-pushed the standardjs branch 2 times, most recently from c4673bc to cbf7112 Compare June 15, 2016 06:03
@gabeio gabeio added the pr label Jun 15, 2016
@dougwilson
Copy link
Contributor

We probably want to merge as many PRs before doing this to this module, otherwise it is going to make it almost impossible to accept any of the current PRs (especially since this module was not using a format even close to standard before). Perhaps part of the 1 -> 2 version of this module? Thoughts?

As for some of the errors, they should be OK if you want to use the template I linked to in the standard issue (the changes do not really load on my phone well, so not sure).

@gabeio
Copy link
Member Author

gabeio commented Jun 16, 2016

Oh that's true we should accept the other pull requests before merging this. I'll update it when we start to push towards v2.

@dougwilson
Copy link
Contributor

Hey @gabeio, if you rebase on top of 16cbfb2 then the __proto__ stuff should be gone now :)

@gabeio
Copy link
Member Author

gabeio commented Jun 17, 2016

Done 😄 yes the __proto__ error is gone 👍

test/session.js Outdated
@@ -1,51 +1,53 @@
/* global describe, it, before */
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than this, why not use the proposed pattern at expressjs/discussions#18 (comment) ? If you don't like the proposed pattern, please speak up in that discussion thread :)!

Copy link
Member Author

@gabeio gabeio Jun 20, 2016

Choose a reason for hiding this comment

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

oh that's fine I just was trying to figure out how to get rid of the standard error (I hadn't realized that explained/showed how to fix that issue!).

@gabeio
Copy link
Member Author

gabeio commented Jun 20, 2016

@dougwilson should I make the changes, just like you made in that commit to this repository also; the package.json & .travis.yml edits that force it to run checks using the eslint?

@dougwilson
Copy link
Contributor

Yep :)

@gabeio
Copy link
Member Author

gabeio commented Jun 22, 2016

done 👍 (three of the tests I had to add an if (err) return done(err))

@dougwilson
Copy link
Contributor

Nice! Doing a rebase should now resolve the second item on the list.

@dougwilson
Copy link
Contributor

And now item 1 should be resolved as well.

@dougwilson
Copy link
Contributor

I also just cherry-picked out your commit gabeio@a893cb3 since that is useful outside of just converting to Standard format.

@gabeio gabeio force-pushed the standardjs branch 3 times, most recently from 62b854c to 2301ff1 Compare June 24, 2016 05:18
@gabeio
Copy link
Member Author

gabeio commented Jun 24, 2016

I rebased and squashed them. (without the commit you cherry-picked 👍 )


I'll continue to rebase as the master updates 👍 .

@gabeio gabeio force-pushed the standardjs branch 3 times, most recently from a4c00e6 to d4313f7 Compare September 9, 2017 03:55
@gabeio
Copy link
Member Author

gabeio commented Sep 9, 2017

@dougwilson there are some new errors/suggestions in the standardjs lint:

/home/travis/build/expressjs/session/index.js
  284:15  error  'new Buffer()' was deprecated since v6. Use 'Buffer.alloc()' or 'Buffer.from()' (use 'https://www.npmjs.com/package/safe-buffer' for '<4.5.0') instead  node/no-deprecated-api
  447:27  error  Unexpected mix of '||' and '&&'  no-mixed-operators
  447:65  error  Unexpected mix of '||' and '&&'  no-mixed-operators
/home/travis/build/expressjs/session/test/session.js
  2350:31  error  Unnecessary escape character: \.  no-useless-escape
  2350:37  error  Unnecessary escape character: \.  no-useless-escape

@dougwilson
Copy link
Contributor

I think I have a stash somewhere with the Buffer change, but not the others.

@gabeio
Copy link
Member Author

gabeio commented Sep 9, 2017

The unnecessary escape character seems possibly incorrect.

@dougwilson
Copy link
Contributor

If the . character is within a character class [ ] then it doesn't need to be escaped, which is probably what the issue is.

@dougwilson dougwilson force-pushed the master branch 2 times, most recently from 9d2e29b to 408229e Compare January 28, 2024 20:24
@bjohansebas
Copy link
Member

Closed due to multiple conflicts, and we haven't chosen which linter to use in the project yet.

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

Successfully merging this pull request may close these issues.

3 participants