search

phetsims / phet-info

Collection of information shared by PhET team members for the purpose of using github effectively and for other process-related topics.
MIT License
59 stars 27 forks source link

What should we do about imports that exceed the 120-char line limit? #139

Closed pixelzoom closed 4 years ago

pixelzoom commented 4 years ago

What should we do about imports that exceed the 120-char line limit?

Slack developer channel:

Jesse Greenberg 7:44 AM WebStorm wants to format this import statment like import EnergySkateParkColorScheme from '../../../../../energy-skate-park/js/energy-skate-park/common/view/EnergySkateParkColorScheme.js'; Is that right? I thought I disabled all auto wrapping but maybe I am missing a setting somewhere?

Sam Reid 8:31 AM checking… 8:33 That happened for me too 8:35 I changed the “Settings=>Editor=>Code STyle => Javascript => Wrapping and Braces => Hard Wrap at” from 120 to 999 and the problem went away: 8:35 image

Jesse Greenberg 8:52 AM Thanks! That fixed it for me too.

Sam Reid 9:27 AM Perhaps we should check in that change to the code style?

Jesse Greenberg 9:40 AM Before checking in, I noticed increasing the "Hard wrap at" setting pushed the visual guide (ruler) out that many characters too. I added another visual guide at 120 characters. I don't want to make everyone's visual guides go away.

Michael Kauzmann:octopus: 10:13 AM Yes I don't think that changing that setting is worth it for just the wrapping import statement.

Chris Malley 10:33 AM The 120-char line limit has not been a hard limit, up to developer discretion, and I'd hate to change that. I see the limit being applied when I auto-import or Organize imports. I do not see the limit applied when I format code. My vote would be to live with it for imports, since (most) are added by the IDE, and typically collapsed.

Jesse Greenberg 10:35 AM Cool, I agree with that and it sounds good to me

zepumph commented 4 years ago

My vote would be to live with it for imports, since (most) are added by the IDE, and typically collapsed.

I'm not sure what @pixelzoom's opinion is from this. Does "live with it" mean to be okay with having a long import split on two lines?

I personally feel like having the "hard wrap" setting in my IDE, and I would rather not turn it off.

pixelzoom commented 4 years ago

Sorry... To clarify, "live with it" means I'm fine with the 120-char line limit being applied to imports "since (most) are added by the IDE, and typically collapsed."

jonathanolson commented 4 years ago

It's tricker to design a lot of automated things (reordering imports, parsing imports for string detection, etc.) around multi-line imports, so some code would need to change. I have a preference to have single-line imports.

samreid commented 4 years ago

I agree, single lines seem preferable for tooling support.

chrisklus commented 4 years ago

From 4/2/20 dev meeting:

We agree single line seems preferable. @jessegreenberg is going to update phet-code-style.xml to not hard wrap at 120, but still have a visual guide at 120 characters. He's also going to create a lint rule that flags two-line import statements.

jessegreenberg commented 4 years ago

This is done. A new lint rule "single-line-import" has been added that enforces this, and above commits outside of chipper were made where the lint rule found import statements on multiple lines. Over slack I said

Hello all - the phet-idea-codestyle.xml was changed for https://github.com/phetsims/phet-info/issues/139, so please be aware of the following.

  • If you use WebStorm/IntelliJ, please pull phet-info, then re-import phet-idea-codestyle.xml so your IDE conforms to this change.
  • A new lint rule was introduced to keep imports on one line.
  • Commits are being pushed to 19 repos with this formatting change.

Closing.

pixelzoom commented 4 years ago

A note about the above commit, for posterity...

I was curious about what specifically changed, so looked at https://github.com/phetsims/phet-info/commit/5e263c1edba5fef6e799a7d5b3c6a94c5685aa15. The entire file had changed! @samreid pointed out that it was a formatting issue. (The formatting of the code-style file is wrong. Oh, the irony.). So I reformatted the file, confirmed that only 2 lines had really changed, and pushed.