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

Sort all the props \o/ #5760

Closed
wants to merge 2 commits into from
Closed

Sort all the props \o/ #5760

wants to merge 2 commits into from

Conversation

willdurand
Copy link
Member

@willdurand willdurand commented Jul 27, 2018

Fix mozilla/addons#12138


  • upgrade eslint-plugin-amo to version 1.5.0
  • apply eslint --fix to automatically sort all the destructured props

@codecov-io
Copy link

codecov-io commented Jul 27, 2018

Codecov Report

Merging #5760 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    mozilla/addons-frontend#5760   +/-   ##
=======================================
  Coverage   97.25%   97.25%           
=======================================
  Files         228      228           
  Lines        5678     5678           
  Branches     1091     1091           
=======================================
  Hits         5522     5522           
  Misses        136      136           
  Partials       20       20
Impacted Files Coverage Δ
src/core/middleware/datadogTiming.js 100% <ø> (ø) ⬆️
src/amo/components/LandingPage/index.js 100% <ø> (ø) ⬆️
src/amo/components/AddonsByAuthorsCard/index.js 100% <ø> (ø) ⬆️
src/amo/components/Home/index.js 100% <ø> (ø) ⬆️
src/ui/components/OverlayCard/index.js 100% <ø> (ø) ⬆️
src/core/reducers/installations.js 100% <ø> (ø) ⬆️
src/disco/utils.js 100% <ø> (ø) ⬆️
src/amo/api/reviews.js 100% <ø> (ø) ⬆️
src/core/installAddon.js 100% <ø> (ø) ⬆️
src/ui/components/HostPermissions/index.js 97.91% <ø> (ø) ⬆️
... and 64 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd69dc0...ac81851. Read the comment docs.

Copy link
Contributor

@kumar303 kumar303 left a comment

Choose a reason for hiding this comment

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

I think this is a terrible idea for the cognitive burden it will impose but if other team members feel it will be helpful, can we at least do it only for multi-line destructors?

For example, this is all on one line so I really don't see why anyone needs to read it in alphabetical order:

const { one, two, three } = stuff;

but multi-line destructors can get quite long and I can see how alphabetizing is more helpful:

const {
  one,
  three,
  two,
} = stuff;

What is the main problem alphabetizing solves? Editors have powerful find features so that doesn't seem like a problem to me.

Copy link
Contributor

@rebmullin rebmullin left a comment

Choose a reason for hiding this comment

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

I don’t have a preference really one way. I just thought that this was the general consensus that this should be done (for readability) so in that way I see how this could be really helpful if this is something we were going to push.

If everyone's okay with the multi-line at least, maybe for now we could start with this?

@willdurand
Copy link
Member Author

can we at least do it only for multi-line destructors?

This would be tricky to automate because ESLint does not really know whether a statement has newlines or not (it is all about the syntax).

@kumar303
Copy link
Contributor

ESLint does not really know whether a statement has newlines or not (it is all about the syntax)

Maybe it could count characters or words to guess whether or not it will span multiple lines?

@willdurand
Copy link
Member Author

I'm going to close this PR for now. I leave the ticket open though.

@willdurand willdurand closed this Aug 8, 2018
@willdurand willdurand deleted the sort-props branch August 8, 2018 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants