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

"space-before-blocks" not working correctly #13553

Closed
Hudda opened this issue Aug 8, 2020 · 16 comments
Closed

"space-before-blocks" not working correctly #13553

Hudda opened this issue Aug 8, 2020 · 16 comments

Comments

@Hudda
Copy link

@Hudda Hudda commented Aug 8, 2020

Tell us about your environment

  • ESLint Version: v7.6.0 (Currently used)
  • Node Version: v12.16.2
  • npm Version: v6.14.4

What parser (default, Babel-ESLint, etc.) are you using?
@typescript-eslint/parser

Please show your full configuration:

Configuration
"space-before-blocks": [
      "error",
      "always"
    ],

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

cacheSkillRights(skillId: string, skillRights: SkillRights): void{
    this.skillRightsCache[skillId] = cloneDeep(skillRights);
  }

What did you expect to happen?
Eslint raise 'space-before-blocks' error as there are no space after void and before '{'

What actually happened? Please include the actual, raw output from ESLint.
Eslint passes successfully

Are you willing to submit a pull request to fix this bug?
No

@Hudda Hudda added bug triage labels Aug 8, 2020
@anikethsaha
Copy link
Member

@anikethsaha anikethsaha commented Aug 8, 2020

Thanks for the report.

The rule is working fine. There is a missing space between void and {, as the rule clearly stats a space before the blocks.
Also, It looks like there should be a rule overriding this rule in @typescript-eslint/eslint-plugin for better TS experience.
Currently, there is no rule for this use case in typescript-eslint that provides TS support.

So maybe it can be a rule request to typescript-eslint.

@Hudda
Copy link
Author

@Hudda Hudda commented Aug 8, 2020

The rule is working fine. There is a missing space between void and {, as the rule clearly stats a space before the blocks.

I think there is a misunderstanding, eslint didn't report any error, but it should because space is missing and I think typescript-eslint won't interfere with any eslint rule until we have the same rule in typescript-eslint, which is not the case here(typescript-eslint do not have space-before-blocks rule).

@anikethsaha
Copy link
Member

@anikethsaha anikethsaha commented Aug 8, 2020

Eslint didnt report because it doesnt know about types. So it won't work properly with typescript. I think typescript-eslint should support this rule.

demo

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Aug 8, 2020

space-before-blocks doesn't control spaces between a keyword and {:

This rule ignores spacing which is between a keyword and a block. The spacing is handled by the keyword-spacing rule

Maybe the @typescript-eslint/keyword-spacing rule could be updated to support this case, if it doesn't already.

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Aug 9, 2020

It's interesting that the rule works well with string, number, and other types because they have Identifier tokens.

void just happens to be a keyword as well. In this context, it's more a type identifier than a keyword, but its token is still Keyword.

We added this check to avoid conflicts with other rules, but there should be no conflicts in this particular case because keyword-spacing checks spacing after void in unary expressions only.

Wondering if we could change the rule to just not check the type of the previous token if the given block is the body of a function declaration/function expression.

@anikethsaha
Copy link
Member

@anikethsaha anikethsaha commented Aug 9, 2020

in this line

if (precedingToken && !isConflicted(precedingToken) && astUtils.isTokenOnSameLine(precedingToken, node)) {

if we change the following,

- if (precedingToken && !isConflicted(precedingToken) && astUtils.isTokenOnSameLine(precedingToken, node)) {
+ if (precedingToken && (['FunctionDeclaration', 'FunctionExpression'].includes(node.type) || !isConflicted(precedingToken)) && astUtils.isTokenOnSameLine(precedingToken, node)) {

the rule will be working fine I suppose, (tests are passing) but still, there would be parsing error for types (using the default parser).

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Aug 9, 2020

Yes, we could add a fixture parser test. I'd also like to fix some things about reporting, and refactor the code to avoid the ASTNode|Token logic in checkPrecedingSpace since it's a bit confusing and error-prone.

@anikethsaha
Copy link
Member

@anikethsaha anikethsaha commented Aug 9, 2020

Yeah,
One thought, why it is a bug? it is intended behavior, right? the parser is working fine just that the code is in TS so it is supposed to break. Are there any cases that is not being tested because of the conflict check and that is not covered in arrow and keyword spacing rules?

or maybe I am missing something?

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Aug 9, 2020

This isn't technically a bug, the rule works fine with JS code.

Core rules may or may not work well with TS code. Most of the rules work perfectly well, those that have some complex issues with TS code are usually extended in the typescript plugin.

The idea here is to just slightly change the logic where it doesn't affect JS code and in a way that doesn't look unreasonable for JS code, but will make the rule work well with TS code. We used to make such small changes before.

@anikethsaha
Copy link
Member

@anikethsaha anikethsaha commented Aug 9, 2020

Coool,

@Hudda
Copy link
Author

@Hudda Hudda commented Aug 10, 2020

Hi guys, so what is the conclusion here, will you implement the functionality? Thanks!

@Hudda
Copy link
Author

@Hudda Hudda commented Aug 13, 2020

@anikethsaha any update on this?

@anikethsaha
Copy link
Member

@anikethsaha anikethsaha commented Aug 13, 2020

I am in support of the change mentioned by @mdjermanovic (that would fix this issue as well. ) .

@eslint
Copy link
Contributor

@eslint eslint bot commented Sep 14, 2020

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@mdjermanovic mdjermanovic self-assigned this Sep 14, 2020
@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Sep 14, 2020

Sorry for the delay! I believe the intention of the keyword check was to avoid conflicts with the keyword-spacing rule, but there is no need to do that for function body blocks. I'll work on this.

@mdjermanovic mdjermanovic reopened this Sep 14, 2020
mdjermanovic added a commit that referenced this issue Sep 23, 2020
@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Oct 19, 2020

Marking this issue and the PR accepted as there are three 👍 from team members.

@mdjermanovic mdjermanovic added accepted and removed evaluating labels Oct 19, 2020
btmills pushed a commit that referenced this issue Oct 24, 2020
…) (#13712)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.