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

Multiple modules from the same import #1165

Closed
jessegreenberg opened this issue Dec 3, 2021 · 8 comments
Closed

Multiple modules from the same import #1165

jessegreenberg opened this issue Dec 3, 2021 · 8 comments
Assignees

Comments

@jessegreenberg
Copy link
Contributor

jessegreenberg commented Dec 3, 2021

Currently, auto-importing modules from the same location results in WebStorm putting them on the same line. There is a setting for this in formatting options. Which do we prefer?

import { KeyboardUtils, Node, SceneryEvent } from '../../../scenery/js/imports.js';

vs

import { KeyboardUtils } from '../../../scenery/js/imports.js';
import { SceneryEvent } from '../../../scenery/js/imports.js';
import { Node } from '../../../scenery/js/imports.js';

If the import line gets too long, WebStorm will break it into multiple lines. But this breaks a lint rule "single-line-import":

import {
  Circle,
  Display,
  DragListener,
  Drawable,
  Rectangle,
  RelativeTransform,
  RichText,
  Text,
  TransformTracker
} from '../../../scenery/js/imports.js';

If we chose to import multiple modules in one import, we should disable the "single-line-import" lint rule.

@jessegreenberg jessegreenberg changed the title Multiple imports from the same module Multiple modules from the same import Dec 3, 2021
@jessegreenberg
Copy link
Contributor Author

Over slack @jonathanolson said

Merging sounds preferable to me, no?

I looked at the lint rule "single-line-import" and it has a line of documentation

Automated tools and processes at PhET assume that imports are on a single line so this is important to enforce.

So maybe we cannot remove this.

@jessegreenberg
Copy link
Contributor Author

jessegreenberg commented Dec 3, 2021

We decided to add this lint rule in phetsims/phet-info#139.

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.

So we should investigate why this isn't happening for TypeScript files/imports.

@jessegreenberg jessegreenberg self-assigned this Dec 3, 2021
@jessegreenberg
Copy link
Contributor Author

The settings we changed in phetsims/phet-info#139 just need to be added back in, it looks like they were removed. Ill reapply them so that imports stay on a single line and we keep our single-line-import eslint rule.

@jessegreenberg
Copy link
Contributor Author

I reported this over slack.

Hello all - I just changed the phet-idea-codestyle.xml so that the formatter does not break long imports into multiple lines and break the "single-line-import" eslint rule. If you use WebStorm you will need to pull phet-info, then re-import phet-idea-codestyle.xml as your style scheme.

This is the same style we applied in phetsims/phet-info#139 so I think this is safe to close.

@jessegreenberg
Copy link
Contributor Author

Over slack @jonathanolson said that maybe we don't need to enforce a single line import any more. Re-opening and adding to the dev meeting agenda for a quick check because there were other voices in phetsims/phet-info#139 that preferred a single line of imports.

If we are OK with default formatting from WebStorm we can delete the "Hard wrap at: 999" workaround and delete our "single-line-import" lint rule.

@jessegreenberg jessegreenberg reopened this Dec 3, 2021
@jonathanolson
Copy link
Contributor

I'm not aware of constraints around import line styles currently. We should ask if anyone else knows something else.

@samreid
Copy link
Member

samreid commented Feb 17, 2022

Single line imports have occasionally been helpful in regexes/searches or search/replace. Is that a good enough reason to keep them as single lines?

@jessegreenberg
Copy link
Contributor Author

Sounds good, I don't think this needs to take dev meeting time. It looks nice as is, closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants