Skip to content
This repository was archived by the owner on Nov 20, 2024. It is now read-only.

Fix directives_ordering for non-pub packages. #2596

Merged
merged 1 commit into from
Apr 19, 2021

Conversation

davidmorgan
Copy link
Contributor

directives_ordering had a gap for google3 because those don't count as pub packages; this seems to do the job.

@google-cla google-cla bot added the cla: yes label Apr 16, 2021
@coveralls
Copy link

coveralls commented Apr 16, 2021

Coverage Status

Coverage decreased (-0.05%) to 94.309% when pulling 8bb4d82 on davidmorgan:directives-ordering-fix into 03fa407 on dart-lang:master.

@@ -271,7 +271,14 @@ class _Visitor extends SimpleAstVisitor<void> {
_checkSectionInOrder(lintedNodes, relativeExports);

var projectName = project?.name;
if (projectName != null) {
if (projectName == null) {
// Not a pub package. Package directives should be sorted in one block.
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to have some tests for this case, but I don't know whether our current test framework supports test cases in non-pub packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks.

I hadn't noticed the test for this rule under integration; now I did notice it and tried to add coverage--but didn't figure out a way to trigger this codepath.

@davidmorgan davidmorgan force-pushed the directives-ordering-fix branch from d7a0bc8 to 8bb4d82 Compare April 19, 2021 07:56
@davidmorgan
Copy link
Contributor Author

Please let me know re: test coverage.

Also, please merge if you want to, I don't have merge privileges here :)

@pq
Copy link
Contributor

pq commented Apr 19, 2021

We can loop back about improved test coverage. I'll land for now. Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

4 participants