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

feat(eslint-plugin): [member-delimiter-style] Add an option 'multilineDetection' to treat types and interfaces as single line if the last member ends on the same line as the closing bracket #2970

Merged
merged 4 commits into from Feb 28, 2021

Conversation

pfgithub
Copy link
Contributor

@pfgithub pfgithub commented Jan 25, 2021

Before this PR:

"member-delimiter-style", {
    singleLine: {delimiter: 'comma', requireLast: false},
    multiLine: {delimiter: 'semi', requireLast: true},
}
type Nested = {inline: true, obj: {
//                         ^
// error: Expected a semicolon.
    inline: false;
} };
//^
//error: Expected a semicolon.

After this PR:

"member-delimiter-style", {
    singleLine: {delimiter: 'comma', requireLast: false},
    multiLine: {delimiter: 'semi', requireLast: true},
    multilineDetection: 'last-member',
}
type Nested = {inline: true, obj: {
    inline: false;
}};

last-member is brackets by default, which keeps existing behaviour

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @pfgithub!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!


🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

@codecov
Copy link

codecov bot commented Jan 25, 2021

Codecov Report

Merging #2970 (17c71a8) into master (1932c8b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2970   +/-   ##
=======================================
  Coverage   92.74%   92.74%           
=======================================
  Files         313      313           
  Lines       10636    10644    +8     
  Branches     3017     3021    +4     
=======================================
+ Hits         9864     9872    +8     
  Misses        353      353           
  Partials      419      419           
Flag Coverage Δ
unittest 92.74% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../eslint-plugin/src/rules/member-delimiter-style.ts 94.91% <100.00%> (+0.47%) ⬆️
...kages/scope-manager/src/referencer/ClassVisitor.ts 91.89% <0.00%> (+0.22%) ⬆️

@bradzacher
Copy link
Member

bradzacher commented Jan 25, 2021

Could you explain more about this and why you want this change?
We generally prefer you raise an issue before just making this kind of breaking change, so that it can be discussed.

I'm not sure I agree that

type T = {a: string, b: {

}};

Should be considered a "single line" type - because it's not really a single line type.

@bradzacher bradzacher marked this pull request as draft January 25, 2021 19:02
@bradzacher bradzacher added the enhancement New feature or request label Jan 25, 2021
@pfgithub
Copy link
Contributor Author

pfgithub commented Jan 25, 2021

The purpose of singleLine member delimiter style is to make single line types not need a trailing delimiter or have a different delimiter

const T = {a: {}}; instead of {a: {};};

When a type spans multiple lines but only its members have newlines, it looks weird for it to use the multiline delimiter style (especially with commas)

const T = {a: {b: {
    c: true,
},},}; // current
const T = {a: {b: {
    c: true,
}}}; // changed

This change won't affect anyone using prettier because prettier doesn't allow code to be formatted like that (link) (although people using prettier probably aren't using member-delimiter-style)

@bradzacher
Copy link
Member

To clarify - I understand the change itself, completely.

I'm trying to understand the use case better. The why.
Why are you formatting your types like this with types defined using different conventions?

This rule has existed for >2 years without this functionality.
Because this is a breaking change (anyone who was using the rule without prettier will now have to change their code), I need a good reason for us to break it.

This change won't affect anyone using prettier because prettier doesn't allow code to be formatted like that

Correct, because prettier enforces a consistent code style in this context.

@pfgithub pfgithub closed this Jan 29, 2021
@pfgithub
Copy link
Contributor Author

pfgithub commented Jan 29, 2021

The eslint comma-dangle property, when set to "always-multiline", doesn't enforce if the last member is on the same line as the }. I have updated this PR to have an option for deciding how to treat this so it doesn't break any existing code.

@pfgithub pfgithub reopened this Jan 29, 2021
@pfgithub pfgithub changed the title feat(eslint-plugin): [member-delimiter-style] Treat types and interfaces as single line if the last member ends on the same line as the closing bracket feat(eslint-plugin): [member-delimiter-style] Add an option 'multilineDetection' to treat types and interfaces as single line if the last member ends on the same line as the closing bracket Jan 29, 2021
@pfgithub pfgithub marked this pull request as ready for review February 2, 2021 19:35
Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

thanks!

This was referenced Mar 15, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants