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

Identify gaps between ESLint rules and JSCS rules #5856

Closed
nzakas opened this issue Apr 14, 2016 · 34 comments
Closed

Identify gaps between ESLint rules and JSCS rules #5856

nzakas opened this issue Apr 14, 2016 · 34 comments
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion documentation Relates to ESLint's documentation enhancement This change enhances an existing feature of ESLint

Comments

@nzakas
Copy link
Member

nzakas commented Apr 14, 2016

As a first step in improving compatibility between JSCS and ESLint, we need to identify the gaps between JSCS rules and ESLint rules. Polyjuice had done some initial analysis that can be found here: https://github.com/brenolf/polyjuice/blob/master/doc/JSCS.md (this is a list of rules and options that ESLint doesn't cover).

I'm not sure what other changes have happened or what version Polyjuice was referring to, so we probably need to double-check that information and see if there are any other gaps.

Let's use this issue to build up a list of missing rules and missing rule options, and then we can file separate issues to implement each of the missing things.

@nzakas nzakas added enhancement This change enhances an existing feature of ESLint documentation Relates to ESLint's documentation accepted There is consensus among the team that this change meets the criteria for inclusion labels Apr 14, 2016
@nzakas nzakas added this to the JSCS Compatibility milestone Apr 14, 2016
@brenolf
Copy link

brenolf commented Apr 16, 2016

As for the first rules added, these were the versions used by Polyjuice:

  • ESLint: v1.7.2
  • JSCS: v2.0.0
  • JSHint: v2.8.0

JSCS has added many rules after that release until v3.0.2, namely:

  • disallowShorthandArrowFunctions
  • disallowArrowFunctions
  • validateOrderInObjectKeys
  • SpacesInsideParenthesizedExpression
  • disallowMultiLineTernary
  • requireMultiLineTernary
  • disallowTabs
  • disallowUnusedParams
  • validateCommentPosition
  • requireSpacesInGenerator
  • disallowIdenticalDestructuringNames
  • disallowNestedTernaries
  • requireSpaceAfterComma
  • disallowUnusedVariables
  • disallowSpacesInsideImportedObjectBraces
  • requireSpacesInsideImportedObjectBraces
  • requireUseStrict
  • requireImportsAlphabetized
  • requireSpaceBeforeDestructuredValues
  • disallowArrayDestructuringReturn
  • requireNewlineBeforeSingleStatementsInIf
  • disallowSpacesInsideTemplateStringPlaceholders
  • requireImportsAlphabetized
  • requireCapitalizedConstructorsNew
  • requireEarlyReturn
  • disallowVar
  • requireArrayDestructuring
  • requireEnhancedObjectLiterals
  • requireObjectDestructuring
  • disallowSpacesInGenerator

I will need to take a look at those to enable Polyjuice to translate them and be compliant with the latest release of both ESLint and JSCS. As for JSHint, no rules were added.

@nzakas
Copy link
Member Author

nzakas commented Apr 16, 2016

Thanks @brenolf ! Some of these we definitely already support. Here are the ones I can think of off the top of my head:

  • disallowShorthandArrowFunctions
  • disallowArrowFunctions - no-restricted-syntax: ["ArrowFunctionExpression "]
  • validateOrderInObjectKeys
  • SpacesInsideParenthesizedExpression - space-in-parens
  • disallowMultiLineTernary
  • requireMultiLineTernary
  • disallowTabs - maybe indent?
  • disallowUnusedParams - covered by no-unused-vars
  • validateCommentPosition
  • requireSpacesInGenerator -maybe generator-star-spacing?
  • disallowIdenticalDestructuringNames
  • disallowNestedTernaries - no-nested-ternary
  • requireSpaceAfterComma - comma-spacing
  • disallowUnusedVariables - no-unused-vars
  • disallowSpacesInsideImportedObjectBraces - object-curly-spacing
  • requireSpacesInsideImportedObjectBraces - object-curly-spacing
  • requireUseStrict - strict
  • requireImportsAlphabetized - sort-imports
  • requireSpaceBeforeDestructuredValues
  • disallowArrayDestructuringReturn
  • requireNewlineBeforeSingleStatementsInIf
  • disallowSpacesInsideTemplateStringPlaceholders
  • requireImportsAlphabetized - sort-imports
  • requireCapitalizedConstructorsNew - new-cap
  • requireEarlyReturn
  • disallowVar - no-var
  • requireArrayDestructuring
  • requireEnhancedObjectLiterals - object-shorthand
  • requireObjectDestructuring
  • disallowSpacesInGenerator

@hzoo
Copy link
Member

hzoo commented Apr 16, 2016

JSCS uses require/disallow so those are probably combined in eslint (for example requireSpacesInGenerator and disallowSpacesInGenerator)

@nzakas
Copy link
Member Author

nzakas commented Apr 18, 2016

Ah right.

@brenolf
Copy link

brenolf commented Apr 27, 2016

Polyjuice v.2.1.0 now parses most rules of that list. Some were left behind, though. You can check the updated list here.

@nzakas
Copy link
Member Author

nzakas commented Apr 27, 2016

@brenolf awesome! Pasted here for easy reference with some notes:

  • CapitalizedComments
  • NewlineBeforeBlockStatements - looks like a similar request in New rule: newline-before-if #5982?
  • ObjectKeysOnNewLine - implementing in New: Add new rule object-property-newline (fixes #5667) #5933
  • PaddingNewLinesAfterBlocks
  • PaddingNewLinesAfterUseStrict
  • PaddingNewLinesBeforeExport
  • PaddingNewLinesBeforeKeywords
  • PaddingNewLinesInObjects
  • SpaceBetweenArguments - seems the same as comma-spacing?
  • SpacesInConditionalExpression
  • SpacesInForStatement
  • SpacesInsideBrackets - maybe array-bracket-spacing and computed-property-spacing?
  • MultiLineTernary
  • SpacesInsideImportedObjectBraces - looks like we do this in object-curly-spacing

@eslint/eslint-team any other thoughts here?

@mikesherov
Copy link
Contributor

There are also configuration value mappings that are missing. For example, the no empty block rule in eslint didn't allow empty catch statements (so it's now optional) where as the default in JSCS allows empty catch statements. That's just one example, but we need to account for this better.

@nzakas
Copy link
Member Author

nzakas commented Apr 28, 2016

Agreed. Any idea how best to track that info?

@mikesherov
Copy link
Contributor

What I did when I first ported all JSHint options to JSCS was create a master ticket with checkboxes. First set of checkboxes should be missing rules, second set is missing rule options.

IIRC, @nzakas you even went a step further and made a google doc. That might be in order for this.

@nzakas
Copy link
Member Author

nzakas commented Apr 28, 2016

A long time ago, I'm not sure it would be relevant now.

Maybe as a first step it would make sense to create a spreadsheet of every JSCS rule and option, and then we can collaboratively fill in compat info?

@mikesherov
Copy link
Contributor

Sorry, I meant spreadsheet and not doc. We're on same page.

@nzakas
Copy link
Member Author

nzakas commented Apr 28, 2016

Do you have an easy way to generate a list of each JSCS rule and its options? I can send you my original spreadsheet, though I think we might want to start fresh with a new list to make sure we aren't relying on old data.

@brenolf
Copy link

brenolf commented Apr 28, 2016

@nzakas I'm afraid to map such rules like SpacesInsideImportedObjectBraces to object-curly-spacing because the latter is much more generic. JSCS already had specific rules for the same objective as object-curly-spacing, so I believe it could be a little bit "loose" to convert that way.

@mikesherov
Copy link
Contributor

object-curly-spacing and SpacesInsideImportedObjectBraces are both misnomers. Named imports are not objects. I'd suggest that the rule be renamed or aliased: object-or-import-curly-spacing

@mikesherov
Copy link
Contributor

@brenolf, IMO, what should happen here is we should map the two and see if anyone complains about wanting two different styles between objects and named imports. My guess is it's effectively zero people.

In general, JSCS was overly specific about customization. I honestly doubt there would be users who want two different styles here.

@brenolf
Copy link

brenolf commented Apr 29, 2016

@mikesherov agreed. There are already some rules in Polyjuice which are "loosely" mapped; One I recall is requireMultipleVarDecl, which is also cited in #4680 (and that's why I was concerned to map SpacesInsideImportedObjectBraces). But I totally agree JSCS is too specific and we could just map mostly with what we have right now.

@nzakas
Copy link
Member Author

nzakas commented Apr 29, 2016

@mikesherov renaming rules is a breaking change, and we generally don't do that unless absolutely necessary. We can definitely investigate aliasing rules, though I'm a bit worried about how confusing that would be users.

My original question stands: is there a way to easily generate a complete list of JSCS rules and options for each? It seems like jscs.info is generated, so maybe this is possible? @hzoo

@mikesherov
Copy link
Contributor

Rule names yes, rule options I don't think so. Thankfully though the setting of config options is isolated in the configure method of each rule, so not totally terrible to do manually.

@hzoo
Copy link
Member

hzoo commented May 3, 2016

@nzakas Right like Mike mentioned, it was only for the rule names since we didn't end up implementing a declarative rule option syntax.

@mysticatea
Copy link
Member

WIP: https://docs.google.com/spreadsheets/d/1-XoW_kb4nU1-Zh_5tvjdgQaeAT9RmKX-H5QZfOizHyY/edit?usp=sharing

@mysticatea
Copy link
Member

Done, but not completely. I'm not familiar with JSCS, so mistakes probably exist.

@hzoo
Copy link
Member

hzoo commented May 3, 2016

Awesome - does look like it covers all the rules, I guess for options we'll have to go through those rather manually

@brenolf
Copy link

brenolf commented May 3, 2016

@mysticatea Long ago I started a list like that, but it's very raw. I believe most of that was what I used in Polyjuice as for JSCS v.2.0.0, so some things have changed in the meanwhile.

PS: If it's interesting somehow here is a similar list with JSHint rules.

@mysticatea
Copy link
Member

I made proposals for red cells in my spreadsheet.

@nzakas
Copy link
Member Author

nzakas commented May 4, 2016

Wow, nice work @mysticatea!

@adamreisnz
Copy link

adamreisnz commented Jun 28, 2016

Hi everyone, as I'm migrating from jscs to eslint I thought I'd mention some rules I come across that I see are missing and don't seem to be mentioned on this page. I'll edit this comment as I find more.

Apologies if you are aware of any of these already.

@platinumazure
Copy link
Member

@adambuczynski Thanks for calling that out! Does issue #3229 cover this?

@adamreisnz
Copy link

@platinumazure yep it seems to indeed!

@platinumazure
Copy link
Member

@adambuczynski Awesome! I see that the issue is already in our JSCS Compatibility milestone, so I believe there's nothing we need to do here (besides actually implementing the change 😄). Thanks again for calling it out!

@pixelpax
Copy link

@mysticatea I'm making the transition from JSCS right now and I found this spreadsheet to be essential. As a fairly new user to both JSCS and ESLint, I found myself trying to wade through the rules page on the site, searching with parameters from jscs which, more often than not, did not return what I needed.

What would be most excellent is if this mapping from JSCS was at least tagged within their corresponding ESLint rule docs, so that when I search for 'SpacesInFunctionExpression', or 'function spaces' or something, I'm can find what I'm looking for; space-before-blocks.

@adamreisnz
Copy link

I actually did it the hard way, and sort of tried to find the rules one by one at first, until I realized it's easier to just go over the ESLInt rules instead and apply what you want from that. I found most rules that I use are already implemented, so I've been able to make a fairly smooth transition. 👍

@ValYouW
Copy link

ValYouW commented Dec 16, 2016

Hi,
After discussing #7739 it seems there is no eslint rule that is like jscs requireNamedUnassignedFunctions. Am I correct or missing something? Thx.

@not-an-aardvark
Copy link
Member

Is there anything left to do for this issue? I'm aware that there are still a few JSCS compatibility issues left (see the project), but it seems like this issue can be closed because we've finished identifying gaps between ESLint rules and JSCS rules.

@kaicataldo
Copy link
Member

I think you're correct. The work for this has been done. Most of the remaining issues are accepted - I'm going to try and take some time to knock them out over the next few weeks.

@not-an-aardvark not-an-aardvark moved this from In Progress to Done in JSCS Compatibility Sep 3, 2017
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Mar 3, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Mar 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion documentation Relates to ESLint's documentation enhancement This change enhances an existing feature of ESLint
Projects
No open projects
Development

No branches or pull requests