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

Release proposal: standard v5 #192

Closed
feross opened this issue Jul 17, 2015 · 38 comments

Comments

@feross
Copy link
Member

commented Jul 17, 2015

eslint v1.0.0 will be released soon! eslint added some new rules that are nice, and lots of existing rules have subtle behavior changes designed to catch more errors.

Let's talk about the next version of standard! We'll bump the major version to standard v5.0.0 to pull in these improvements.

New rules:

There were other new rules that I didn't add because they have too many false positives, require ES6-only codebases, or would be divisive: standard/eslint-config-standard@a211a1c

Changed rules:

  • use no-extra-parens instead of the deprecated no-wrap-func rule (standard/eslint-config-standard@fc8a076)
  • indent got stricter and catches errors in object literal indentation. 12/131 repos in the test suite started failing after this rule was improved.

I'm posting this to get feedback. Please share your thoughts! I published a prerelease version (v5.0.0-1). Please give it a try on your code!

npm install -g standard@next
@feross

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2015

This might also be a good opportunity to make other breaking changes we want to make. In particular, the work in this PR: #189 Anything else we should think about?

@jprichardson

This comment has been minimized.

Copy link
Member

commented Jul 17, 2015

Unnecessary apply (no-useless-call) rule fails on common case of using concat to flatten an array one level deep. Consider:

var arr = [].concat.apply([], [[1], [2], [3]])
console.dir(arr)
@Flet

This comment has been minimized.

Copy link
Member

commented Jul 17, 2015

standard@5.0.1-0 === standard@latest?

@feross

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2015

@Flet I believe that's expected behavior. The latest tag is what you use if you want prerelease versions. Running npm install -g standard still gives you the stable 4.5.4 version.

@feross

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2015

@jprichardson Good catch re: no-useless-call. This looks like an eslint bug to me. I just opened an issue: eslint/eslint#3054

I also just learned that5.0.1-0 should have been 5.0.0-1. Future v5 prereleases will be 5.0.0-1, 5.0.0-2, 5.0.0-3, etc.

@Flet

This comment has been minimized.

Copy link
Member

commented Jul 17, 2015

@feross thanks, I did not realize this!

Doing this: npm install -g standard= 4.5.4
Doing this: npm install -g standard@latest = 5.0.1-0

Looking for some npm docs on this behavior, but not finding any :(

Anyways, carry on. :)

@feross

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2015

@Flet hm, actually, i think what's happening is that when you do a publish, npm updates the latest tag to point to it. and npm install just picks the most recent, non-prerelease version to install.

@Flet

This comment has been minimized.

Copy link
Member

commented Jul 17, 2015

Yeah, I found where npm iterates over the versions and picks the one to install... It looks like semver is checking the version against an empty string since none was specified.
Also https://github.com/npm/node-semver#prerelease-tags

Fun stuff!

❯ npm install semver && node 
> var semver = require('semver')
undefined
> semver.satisfies('5.0.0-0', '')
false
> semver.satisfies('5.0.0', '')
true

And of course there is code above this part that will use the tagged version if a tag is passed.

Cool, I assumed that lastest was always used, but that is not the case!

@LinusU

This comment has been minimized.

Copy link
Member

commented Jul 18, 2015

I get this version when I run the following, is that suppose to happen?

➜  npm install --save-dev standard
standard@5.0.0-2 node_modules/standard
@dcousens

This comment has been minimized.

Copy link
Member

commented Jul 19, 2015

@jprichardson because it should?

[].concat([1], [2], [3]) will be exactly the same.

@jprichardson

This comment has been minimized.

Copy link
Member

commented Jul 20, 2015

[].concat([1], [2], [3]) will be exactly the same.

No it's not. That outputs:

[ 1, 2 ]

What I was trying to demonstrate:

var b = [[1], [2], [3]]
var arr = [].concat.apply([], b)
console.dir(arr)

Which standard accepts. So maybe there isn't much of an issue here.

@dcousens

This comment has been minimized.

Copy link
Member

commented Jul 20, 2015

No it's not. That outputs: [1, 2]

> [].concat([1], [2], [3])
[ 1, 2, 3 ]

@jprichardson, huh?

I use this pattern all the time to flatten arrays, using [].concat(... arrays).
The arity of the arguments is not important.

edit: Just re-read your example, you meant in the case that b wasn't a constant expression... I'm pretty sure it will be allowed in that case.

@jprichardson

This comment has been minimized.

Copy link
Member

commented Jul 20, 2015

@jprichardson, huh?

Had a screw loose, you are correct.

I use this pattern all the time to flatten arrays, using [].concat(... arrays).

As do I, that's why I brought up the issue as noted in my first comment.

Just re-read your example, you meant in the case that b wasn't a constant expression... I'm pretty sure it will be allowed in that case.

Hence, why I said:

Which standard accepts. So maybe there isn't much of an issue here.

i.e. it looks like standard/eslint is working correctly :)

@feross

This comment has been minimized.

Copy link
Member Author

commented Jul 20, 2015

@LinusU When I run npm install --save-dev standard I get:

standard@4.5.4 node_modules/standard

Can you check your package.json and make sure that "standard": "*" isn't present? npm install should not install prerelease versions like 5.0.0-2 unless the package.json explicitly allows it. I think that is what's going on for you.

@LinusU

This comment has been minimized.

Copy link
Member

commented Jul 20, 2015

screen shot 2015-07-21 at 00 30 51

➜  test  npm --version 
2.10.1
➜  test  node --version 
v0.12.4
@dmonad

This comment has been minimized.

Copy link

commented Jul 21, 2015

generator functions should always have a yield keyword (standard/eslint-config-standard@63ab1c6)

I agree that this is a neat rule in most cases. But what if a third party library expects a generator function as an argument, and my generator implementation doesn't actually need to yield? Since I can't configure standard it is impossible for me to lint successfully.

@dcousens

This comment has been minimized.

Copy link
Member

commented Jul 21, 2015

@DadaMonad then use an inline comment to disable that rule in place? Since, it would seem, in that case, you know that it is OK.

@dmonad

This comment has been minimized.

Copy link

commented Jul 21, 2015

@dcousens I don't think that this is a good solution. I have plenty of generator definitions that do not yield. In my humble opinion, the linter should not get too much in my way.

Furthermore, I don't really see a scenario where a rule like this could prevent me from doing something stupid. When you declare a generator function you probably have good reasons for it..

@dcousens

This comment has been minimized.

Copy link
Member

commented Jul 21, 2015

@DadaMonad well, beyond that, I haven't used yield enough to give a meaningful opinion :), so I'll trust your judgement.

@feross

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2015

@LinusU

Okay, I cleared up the version confusion by using tags.

$ npm dist-tag ls standard
latest: 4.5.4
next: 5.0.0-3

To get the 5.0.0 beta versions, use npm install standard@next.

@feross

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2015

@DadaMonad Can you link to some examples of generator functions you've written that do not yield? You may have a good point on this.

@dmonad

This comment has been minimized.

Copy link

commented Jul 22, 2015

@feross I want my framework to support different databases to store its state persistently. I use indexedDB in the browser, and redis in nodejs. When I yield, I request data from the database. You can also store non-persistently in memory. An in-memory database implementation naturally has no asynchronous behaviour - therefore, I do not need to yield.

You can compare the following files: Memory vs. indexedDB

@mightyiam

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2015

Thank you for 5.0!

feross added a commit to standard/eslint-config-standard that referenced this issue Jul 24, 2015

@feross

This comment has been minimized.

Copy link
Member Author

commented Jul 24, 2015

@DadaMonad Thanks for the feedback. I just removed the require-yield rule in 5.0.0-5.

@dmonad

This comment has been minimized.

Copy link

commented Jul 24, 2015

Thanks a lot @feross ! 👍

@cesarandreu

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2015

Another case where you want a generator function that doesn't yield anything is with koa. I have a reasonable amount of those. Each middleware is a generator functions, and depending on how you setup your middleware, your final "controller action" middleware can end up looking like this:

UsersController.current = function * current () {
  this.body = this.state.user
}

Along the same line, when using async functions you don't wanna force it to have an await. One example of this: you have an async function that'll return a cached value immediately if it's available, otherwise it will do a network request. Since an async function will always return a promise, it's always async (no zalgo, kthx) but you might not have anything that you need to actually await.

async function getOrFetch (id) {
  return id in cache
    ? cache[id]
    : (await fetchThing(id))
}
@feross

This comment has been minimized.

Copy link
Member Author

commented Jul 27, 2015

@cesarandreu Good to know!

@feross

This comment has been minimized.

Copy link
Member Author

commented Aug 3, 2015

ESLint has one more breaking change we haven't discussed yet:

Switch to 1-based columns (fixes eslint/eslint#2284) (Nicholas C. Zakas)

Before, ESLint would report errors with 0-based columns.

The current 5.x pre-releases are just passing this change through, meaning that editor integrations are currently off-by-one with standard 5.x. It's not hugely disruptive, but plugin maintainers should update their packages.

That's you @Flet, @ricardofbarros, @ishamf, @scrooloose. 🐈 😄

I'll release the stable 5.0.0 today.

@LinusU

This comment has been minimized.

Copy link
Member

commented Aug 3, 2015

@feross If plugin maintainers need to make an update, what do you think of adding --json to get the output in a format even easier to parse than what is current. That way we have a path forward for starting to change the output to be even prettier and more human readable.

ping #218

@feross

This comment has been minimized.

Copy link
Member Author

commented Aug 3, 2015

@LinusU The current format is super easy to parse with a one-line regex https://github.com/Flet/SublimeLinter-contrib-standard/blob/master/linter.py#L25 But let's keep that discussion in #218.

@feross

This comment has been minimized.

Copy link
Member Author

commented Aug 3, 2015

standard 5.0.0 is released! 🎉 🎈

@feross feross closed this Aug 3, 2015

@ricardofbarros

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2015

Thank you @feross Will update as soon as possible 😄

@feross

This comment has been minimized.

Copy link
Member Author

commented Aug 4, 2015

@cesarandreu

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2015

\o/

@mightyiam

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2015

Hurrah!

@boennemann

This comment has been minimized.

Copy link

commented Aug 5, 2015

@feross Thanks so much for this new version!

I just wanted to suggest adding these release notes (or at least a link) to the Releases page on GitHub or to a CHANGELOG.md file.

For me I only notice that there is a new version of standard, because some dependency badges turn yellow or red. I then come to your repo page to check whether the changes actually affect me and how to update, but I just can't find any indication of what's breaking. Only because I know you and the project well enough I then turn over to Twitter where I find a link to this issue. I think using common places like GitHub Releases or CHANGELOG.md makes the update process way easier for everyone.

Thank you 👍

@LinusU

This comment has been minimized.

Copy link
Member

commented Aug 5, 2015

+1 for keeping a changelog, it's a great way to keep up to date with the rule changes.

@feross

This comment has been minimized.

Copy link
Member Author

commented Aug 5, 2015

@boennemann @LinusU Thanks for the changelog suggestion. I just added the release notes from this issue to the Releases page. 👍

@feross feross changed the title standard v5.0.0 Release proposal: standard v5 Jul 12, 2016

@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
10 participants
You can’t perform that action at this time.