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

Flow types support in the order rule #732

Closed
wants to merge 1 commit into from
Closed

Flow types support in the order rule #732

wants to merge 1 commit into from

Conversation

avaly
Copy link

@avaly avaly commented Jan 28, 2017

Closes #645

PS: I also included a yarn.lock file: https://yarnpkg.com/en/docs/version-control

Please autosquash my fixup commits before merging. 😄

@coveralls
Copy link

coveralls commented Jan 28, 2017

Coverage Status

Coverage increased (+0.009%) to 94.921% when pulling 0cf4653 on avaly:order-flow-types into 790dd66 on benmosher:master.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Adding yarn.lock should be a separate change (one I disagree with, not the least of which is because this isn't a top-level app). Would you mind separating it into a separate PR?

@avaly
Copy link
Author

avaly commented Jan 28, 2017

I removed the yarn.lock file.

@coveralls
Copy link

coveralls commented Jan 28, 2017

Coverage Status

Coverage increased (+0.009%) to 94.921% when pulling bd16b98 on avaly:order-flow-types into 790dd66 on benmosher:master.

@coveralls
Copy link

coveralls commented Jan 28, 2017

Coverage Status

Coverage increased (+0.009%) to 94.921% when pulling bd16b98 on avaly:order-flow-types into 790dd66 on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.3%) to 93.623% when pulling ecc0fcb on avaly:order-flow-types into 790dd66 on benmosher:master.

@coveralls
Copy link

coveralls commented Jan 28, 2017

Coverage Status

Coverage increased (+0.009%) to 94.921% when pulling ecc0fcb on avaly:order-flow-types into 790dd66 on benmosher:master.

@jfmengels
Copy link
Collaborator

Thanks a lot for your work on this @avaly!

Since the issue was created, Flow v0.38 came out and allowed importing both types and values in a single import statement, which sounds like a way better syntax to me and would make this new type less useful.

I described my concerns and propositions on the original issue, let me know what you think #645 (comment)

@avaly
Copy link
Author

avaly commented Jan 31, 2017

@jfmengels I was aware of the new syntax in Flow 0.38, however I still feel this PR is relevant. Since the old syntax to import Flow types is still available, this rule should at least offer the opportunity to configure the code style some may want for their projects. Personally, I still prefer having all the Flow types imports separate from the normal imports.

@avaly
Copy link
Author

avaly commented Feb 14, 2017

Any more thoughts on this? I would really like to have this available.

@jfmengels
Copy link
Collaborator

The order rule is about the imported module, not what is being imported. I'm pretty sure that if we add the type/flow import type, then we'd soon have a request to order the type imports with the same logic (so you'd need subtypes like flow parent, flow external, ...), which would make sense because that is the purpose of this rule to start with, and things will start to get messy.
Having this new feature in Flow 0.38 is a fortunate and welcome solution to this problem IMO.

I don't want to tell you and other users how to import your types, but I really don't think this is the best way to handle this... I'm open to comments though.

Please note that if you really feel like going this path anyway, you always have the option to fork this plugin and create a new package with the forked order rule.

(cc @gajus who opened the original issue, if you feel like commenting)

@avaly
Copy link
Author

avaly commented Mar 9, 2017

I'll close this PR and fork this rule into a separate package.

@avaly avaly closed this Mar 9, 2017
@coveralls
Copy link

coveralls commented Mar 9, 2017

Coverage Status

Coverage decreased (-2.6%) to 92.364% when pulling 2742ca5 on avaly:order-flow-types into 98e7048 on benmosher:master.

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

Successfully merging this pull request may close these issues.

Add type import support to import/order
4 participants