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

Attributes on first import don't work well with webstorm actions #517

Closed
xvrh opened this issue Dec 23, 2016 · 8 comments
Closed

Attributes on first import don't work well with webstorm actions #517

xvrh opened this issue Dec 23, 2016 · 8 comments

Comments

@xvrh
Copy link

xvrh commented Dec 23, 2016

When I write:
screenshot320

And later, I want to use a Future:
screenshot321

This result in:
screenshot322

See that dart:async is imported before the @TestOn. Which has no effect now.

This is even more a problem when the editor collapse your imports by default:
screenshot323

The same problem can arise if I use the "Sort members in Dart File" feature of Webstorm.

Any chance to be able to write the attributes directly on the main method?
screenshot325

I know that I can write a library directive but it's really a pain.

@alexander-doroshko
Copy link

I see a conceptual problem with file-level annotations as how they are used in the package:test.

Dart language spec says that metadata is associated with the abstract syntax tree of the program construct that immediately follows the metadata. With the example above @TestOn belongs to import 'package:test/test.dart. Metadata + import statement is parsed as a single AST node.

But testing framework treats it as a file-level (precisely: library-level) metadata. There's no such concept in the language spec.

I'm not sure if any reasonable solution is possible because of this contradiction. When the Analyzer alpha-sorts directives or inserts a directive because of a quick fix it has no right to detach metadata from one directive and attach it to another one.

One more use case: if the first import is unused and you run 'Optimize directives' Analyzer completely removes the unused import together with its metadata. And I can't say that Analyzer is doing wrong according to the spec.

Just my $0.02.

@kevmoo
Copy link
Member

kevmoo commented Dec 23, 2016

I've chatted a bit w/ @nex3 about this.

I think this is a case where having a library tag – just to hang the annotation off – is a good call.

I get the same issue when doing 'cleanup imports' in intelliJ.

@matanlurey
Copy link
Contributor

@kevmoo Any reason having it on main won't do? It seems more idiomatic.

@nex3
Copy link
Member

nex3 commented Dec 27, 2016

You can always add a library tag to make the syntax match up with the semantics if that's what you want—although it might also be a good idea to file an issue against WebStorm to handle this, since test is a common library.

Any reason having it on main won't do? It seems more idiomatic.

There are a number of reasons. It's slower to parse, main() may not be syntactically defined in the library in question, and having something at the very top of a file makes it very visible which platforms that suite can be run on.

@alexander-doroshko
Copy link

it might also be a good idea to file an issue against WebStorm to handle this

It is not an IDE that is doing this, it is a Dart Analysis Server from the Dart SDK, IDE only renders what server says.
And I do not see how it could be fixed at Analysis Server end. Currently it conforms the Dart Language Specification and in case of any change it won't conform.

@nex3
Copy link
Member

nex3 commented Dec 27, 2016

Ah, good point. That's too bad.

@alexander-doroshko
Copy link

alexander-doroshko commented Dec 27, 2016

Yes, as already mentioned above, import statement metadata should be associated only with the import statement according to spec, but package:test treats it as a file-level entity.

Restricting test-specific annotations to the library statements only would probably be a solution. And it would look better from the conceptual point of view: library statement is the entity that describes the whole file (or more than one in case of parts), and test-specific metadata has an influence on the whole file.

Unfortunately such solution is backward-incompatible.

@nex3
Copy link
Member

nex3 commented Dec 27, 2016

I don't think there's a lot of value in adding restrictions here anyway, even if it weren't backwards-incompatible. I'm happy to leave it up to individual authors to decide if they prefer having a library tag or not.

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

No branches or pull requests

5 participants