amatiasq / vsc-sort-imports

Sort ES6 imports automatically.
ISC License
58 stars 24 forks source link

The imports are not sorted correctly #20

Closed ankurarora closed 4 years ago

ankurarora commented 6 years ago

Hi,

I have flow at top with JSX classes and styles imported. When I save my file this is what I am getting.

Before Sort

// @flow
import Reboot from 'material-ui/Reboot';
import React from 'react';

import keenImage from '../assets/images/logo.png';

// import sass styles
import '../css/main.scss';

import Footer from '../components/footer/Footer';
import Header from '../components/header/Header';

After Sort Applied

// import sass styles
import '../css/main.scss';

// @flow
import Reboot from 'material-ui/Reboot';
import React from 'react';

import keenImage from '../assets/images/logo.png';
import Footer from '../components/footer/Footer';
import Header from '../components/header/Header';

// @flow has been pushed down, I see that as a problem

amatiasq commented 6 years ago

I see now. That's actually an intended feature. A comment above a import declaration is considered a comment related to that import so it is moved with the import it's supposed to be related to. I thought you meant the extension was moving the //@flow comments below all import declarations, now I see it's not the case.

In any case I understand this can be annoying but this should happen only once per file, right? if you move it back to the top again it won't move it back down, right?

We can try to add an exception for a comment matching exactly // @flow but exceptions is not something we want to go with and comes with it's own issues.

What do you think?

ankurarora commented 6 years ago

Instead of looking at // @flow, how about disabling a line to move if it has some special comment for e.g. // @flow ignore-sort-imports? Using this it will give an option to stop moving lines which has that comment at end of line.

If you want I can spend time on it and try to make it to work.

cliffkoh commented 6 years ago

Hi @ankurarora, I strongly recommend just leaving a blank line between // @flow and the first import statement. This will fix the problem entirely while allowing you to use the extension. Let me know if there are some reasons you're strongly opposed to doing this though...

callumlocke commented 6 years ago

I think it would be good for this VSCode plugin to support it automatically.

This plugin is great, but its basic purpose is to provide a shortcut for a repetitive task. It should 'just work' in as many cases as possible, otherwise I might as well go back to manually sorting imports. It's a handy, niche editor plugin at the end of the day, and Flow is a major, widespread tool. The suggestion that Flow users should introduce and remember new code style rules for compatibility with this plugin is a non-starter in my opinion. The problem here is that vsc-sort-imports mangles a widely used tooling directive (// @flow).

ankurarora commented 6 years ago

Adding an extra line is not a problem but in technical terms it is just a comment in the code, I think it would be good for this extension to ignore any such comments while sorting import statements

callumlocke commented 6 years ago

If ignoring them means never moving comment lines from the top, that's fine with me.

amatiasq commented 6 years ago

"Ignoring" is an easy word with a not-so-clear functionallity.

This extension is rewriting the import section so what does ignore mean in this context?

The only way to actually ignore them is to omit them on the rewrite, efectively removing them.

If we move them to the top we are not ignoring them. If we move them to the bottom we aren't ignoring them either.

When this issue appear on the development phase I found more useful to move comments directly above a import statement with that import statement. The comment being directly above it usually means it's related to it.

ankurarora commented 6 years ago

with "ignoring" I meant specifically the comments from the top i.e. before first import

cliffkoh commented 6 years ago

The counterargument for the current behavior is that folks sometime want to document an import. Scenarios might include documenting why something that is not directly referenced is being imported. Ignoring all top-of-file comments will break other people in this scenario.

However, as a compromise, one thing we can do is allow this to be a user preference setting. If a workspace uses flow extensively and does not care for preserving comments on import statements, we can skip the first n lines where n = number of lines of comments at the top of the file.

Contributions welcome ^_^

wootwoot1234 commented 5 years ago

@amatiasq thanks for this great tool!

@ankurarora, your 2-star review (here) seems a little harsh, right? Please consider updating to at least 4 stars. @amatiasq has clearly put in a lot of work on his own time to get this to where it is.

amatiasq commented 5 years ago

@wootwoot1234 Thanks for the appreciation, keep in mind that @peterjuras made most of this project from start :)

Reviews are what they are, everyone is free to evaluate as they feel but thank you so much for the effort 👍

amatiasq commented 4 years ago

Closed since solutions have been provided and discussion is over.