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

style: Enable ESLint and Prettier for all files #2288

Closed
wants to merge 1 commit into from

Conversation

nschonni
Copy link
Contributor

@nschonni nschonni commented Sep 28, 2022

This is a little big, so I can try and break it down a bit, if the end result is desired:

  • Remove ESLint rules that overlap with Prettier/EditorConfig
  • Remove Prettier rules overlapping with EditorConfig
  • Replace Prettier-eslint with plugin/config versions
  • Set Environment to ES2022 and remove globals that are not required
  • Remove babel parser, since covered by ES2022
  • Ignore WASM folders because of top level awaits
  • Rename script commands to match mdn/content

@github-actions
Copy link

This pull request has merge conflicts that must be resolved before it can be merged.

@NiedziolkaMichal
Copy link
Member

I can see few problems with this commit:

  • Unfortunately it's going to change height of multiple examples. For example Array.reduceRight will be converted from smaller to medium. Basic solution is to make separate PR in content repository, in which we revisit specified heights of all interactive examples modified in this PR. Alternative solution is to wait for #839 & #7679 to be deployed, which will make it no longer necessary to update content repo.

  • Comments in examples of bitwise operators are no longer aligned. For example take a look at &=. Currently it's very easy to see how individual bits are affected, which makes those operators easier to be understood.

  • Forced line breaks in arrays will make some examples huge. For example take a look at extends or NumberFormat.

  • I feel code style would be better without line wrapping like in examples TypedArray.at or Symbol.split, but that's just my opinion.

  • I feel unnecessary double parenthesis in examples like exponentiation assignment might be confusing to the user.

Except for that I can see clear benefits of this PR. Most of the examples would look better with those changes applied.

@github-actions
Copy link

This pull request has merge conflicts that must be resolved before it can be merged.

@schalkneethling
Copy link

Hey @nschonni, thoughts on the comments here #2288 (comment)

@github-actions
Copy link

github-actions bot commented Dec 6, 2022

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions
Copy link

github-actions bot commented Dec 8, 2022

This pull request has merge conflicts that must be resolved before it can be merged.

@schalkneethling
Copy link

Hey @nschonni and @NiedziolkaMichal,

First, thank you for the work you have done here, Nick. There were a couple of items brought up by Michal here #2288 (comment)

Please review them when you have a moment. Also, there are a couple of conflicts at the moment, mainly in package.json. Thanks so much.

@nschonni
Copy link
Contributor Author

The config does have overrides for particular files

"overrides": [

If you've got suggestions for those, then I can add them, but I didn't see anything actionable for me from the comments above

@github-actions
Copy link

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions
Copy link

This pull request has merge conflicts that must be resolved before it can be merged.

@schalkneethling
Copy link

Hey @nschonni and @NiedziolkaMichal,

We had a chat about this one. The work here is great and something we want but, the following topic raised by Michal is a blocker at the moment.

Unfortunately it's going to change height of multiple examples.

We have brought this to the attention of engineering and will follow up on this in the new year. We are also discussing whether this can be something the community can assist with if we have low-bandwidth internally.

We hope this makes sense and is acceptable to you all. Thanks again for the work done.

@github-actions
Copy link

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions
Copy link

This pull request has merge conflicts that must be resolved before it can be merged.

- Remove ESLint rules that overlap with Prettier/EditorConfig
- Remove Prettier rules overlapping with EditorConfig
- Replace Prettier-eslint with plugin/config versions
- Set Environment to ES2022 and remove globals that are not required
- Remove babel parser, since covered by ES2022
- Ignore WASM folders because of top level awaits
- Rename script commands to match mdn/content
@github-actions
Copy link

github-actions bot commented Jan 3, 2023

This pull request has merge conflicts that must be resolved before it can be merged.

@schalkneethling
Copy link

@NiedziolkaMichal, @bsmth, anyone opposed to us landing this one?

@bsmth
Copy link
Member

bsmth commented Jan 9, 2023

I think the changes are good, but it might be best if these two PRs land first so we don't introduce display issues:

@schalkneethling
Copy link

schalkneethling commented Jan 16, 2023

I think the changes are good, but it might be best if these two PRs land first so we don't introduce display issues:

I have assigned reviewers for these pull requests.

@github-actions github-actions bot added the idle Issues and pull requests with no activity for three months. label Mar 11, 2023
@github-actions github-actions bot removed the idle Issues and pull requests with no activity for three months. label May 5, 2023
@github-actions github-actions bot added the idle Issues and pull requests with no activity for three months. label Jun 5, 2023
@nschonni nschonni closed this Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
idle Issues and pull requests with no activity for three months. merge conflicts 🚧
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants