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

[eslint] Enforce operator-linebreak and no-multiple-empty-lines #3516

Merged
merged 1 commit into from
Mar 6, 2016
Merged

[eslint] Enforce operator-linebreak and no-multiple-empty-lines #3516

merged 1 commit into from
Mar 6, 2016

Conversation

oliviertassinari
Copy link
Member

No description provided.

@@ -51,6 +52,7 @@ rules:
no-var: 2
object-curly-spacing: [2, never]
one-var: [2, never]
operator-linebreak: [2, "after"]
Copy link
Member

Choose a reason for hiding this comment

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

Could we do this: [2, "after", { "overrides": { "?": "before", ":": "before"} }] to promote this style:

const foo = open
  ? 'open'
  : 'close';

this style is a lot cleaner for ternary, and a lot more readable when it's nested:

const foo = halfOpen
  ? 'halfOpen'
  : open
    ? 'open'
    : 'close';

Copy link
Member

Choose a reason for hiding this comment

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

Absolute not! 😆

Copy link
Member

Choose a reason for hiding this comment

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

😅

Copy link
Member Author

Choose a reason for hiding this comment

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

I would rather keep the style consistent and do no use the overrides option.
What do we do here 😁?

Copy link
Member

Choose a reason for hiding this comment

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

Carry on, nevermind my comment. As long as there is consistency I'm happy 😍 😍

Copy link
Member Author

Choose a reason for hiding this comment

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

Awesome and I'm already done 😁.

@alitaheri
Copy link
Member

I think this is safe to merge. 👍 Let's do it 😎

@nathanmarks
Copy link
Member

@mbrookes mbrookes added PR: Review Accepted on hold There is a blocker, we need to wait and removed PR: Needs Review labels Mar 2, 2016
@mbrookes
Copy link
Member

mbrookes commented Mar 2, 2016

Holding for PR catch-up.

@oliviertassinari
Copy link
Member Author

I think that it's safe to merge this PR. We haven't seen any conflict in a week, while merging many PRs.

@nathanmarks nathanmarks removed the on hold There is a blocker, we need to wait label Mar 6, 2016
@nathanmarks
Copy link
Member

@oliviertassinari I'm going to re-run the build and will merge if all is 👍

@nathanmarks nathanmarks added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Review Accepted labels Mar 6, 2016
@nathanmarks
Copy link
Member

Ok, travis GUI output was screwed up at first (seemed cached from the initial build actually). I deleted my posts regarding it and the raw log. The errors are visible in the gui now.

@oliviertassinari can you fix these up quickly:

src/Stepper/HorizontalStep.jsx
  115:8  error  '?' should be placed at the end of the line  operator-linebreak
  116:8  error  ':' should be placed at the end of the line  operator-linebreak
  117:8  error  '?' should be placed at the end of the line  operator-linebreak
  118:8  error  ':' should be placed at the end of the line  operator-linebreak
src/Stepper/Stepper.jsx
  123:9  error  '&&' should be placed at the end of the line  operator-linebreak
src/Stepper/VerticalStep.jsx
  205:8  error  '?' should be placed at the end of the line  operator-linebreak
  206:8  error  ':' should be placed at the end of the line  operator-linebreak
  207:8  error  '?' should be placed at the end of the line  operator-linebreak
  208:8  error  ':' should be placed at the end of the line  operator-linebreak

@nathanmarks nathanmarks self-assigned this Mar 6, 2016
@oliviertassinari oliviertassinari added PR: Needs Review and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Mar 6, 2016
@nathanmarks nathanmarks added PR: Review Accepted PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Mar 6, 2016
? theme.hoveredAvatarColor
: theme.inactiveAvatarColor);
((isActive || isCompleted) ?
theme.activeAvatarColo :
Copy link
Member

Choose a reason for hiding this comment

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

Is this a mistake?

We need tests to catch this sort of stuff so we can work with confidence. We shouldn't need to manually review typos like this 😢

Copy link
Member Author

Choose a reason for hiding this comment

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

😨

Copy link
Member

Choose a reason for hiding this comment

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

😨

@nathanmarks nathanmarks added PR: Review Accepted and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Mar 6, 2016
nathanmarks added a commit that referenced this pull request Mar 6, 2016
…ty-lines

[eslint] Enforce operator-linebreak and no-multiple-empty-lines
@nathanmarks nathanmarks merged commit b4cc161 into mui:master Mar 6, 2016
@oliviertassinari oliviertassinari deleted the eslint-no-multiple-empty-lines branch March 6, 2016 15:38
mbradleyis pushed a commit to staticinstance/material-ui that referenced this pull request Mar 7, 2016
…empty-lines

[eslint] Enforce operator-linebreak and no-multiple-empty-lines
@zannager zannager added package: system Specific to @mui/system package: eslint Specific to eslint-plugin-material-ui labels Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: eslint Specific to eslint-plugin-material-ui package: system Specific to @mui/system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants