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

Fix directives_ordering for a leading dot in path #4538

Merged
merged 1 commit into from
Jul 12, 2023
Merged

Conversation

oprypin
Copy link
Contributor

@oprypin oprypin commented Jul 6, 2023

It sorts imports as follows:

import './foo1.dart';
import '../../foo2.dart';
import '../foo3.dart';
import 'foo4.dart';

This change corrects it to the following:

import '../../foo1.dart';
import '../foo2.dart';
import './foo3.dart';
import 'foo4.dart';

Fixes #2678

See #2678 (comment) for an explanation on how this edit relates to the behavior

It used to sort imports as follows:

    import './foo1.dart';
    import '../../foo2.dart';
    import '../foo3.dart';
    import 'foo4.dart';

This change corrects it to the following:

    import '../../foo1.dart';
    import '../foo2.dart';
    import './foo3.dart';
    import 'foo4.dart';
@coveralls
Copy link

coveralls commented Jul 6, 2023

Coverage Status

coverage: 96.764% (+0.001%) from 96.763% when pulling 0e41c01 on directives_dot into 45632a8 on main.

@bwilkerson
Copy link
Member

Does this cause the lint to diverge from the 'Sort Imports' operation?

@oprypin
Copy link
Contributor Author

oprypin commented Jul 6, 2023

I am not fully sure this is a valid confirmation, but after I edit the lint in google3, the behavior of the bulk fix for the lint changes accordingly. So I think it means that that operation's behavior just uses the same code that is in this lint.

@bwilkerson
Copy link
Member

I don't know how that would happen. The code in the organize imports implementation says

  /// Should keep these in sync! Copied from
  /// https://github.com/dart-lang/linter/blob/658f497eef/lib/src/rules/directives_ordering.dart#L380-L387
  /// Consider finding a way to share this code!

@oprypin
Copy link
Contributor Author

oprypin commented Jul 6, 2023

I see. Yes, I realize my validation was not right. It's not that the fixer was doing the right thing now, it's just that it wasn't kicking in anymore.
Then how can we proceed and fix the code in both places?

@bwilkerson
Copy link
Member

I can only think of two approaches (though there are probably others).

  1. First update both the linter and the server to use the same implementation of this method, as per the comment in the organize imports implementation, then modify the shared implementation.

  2. Update the server's implementation at the same time we roll a new version of the linter into the SDK. This will only work if we ensure that the internal version of the linter is always the version in the SDK.

@oprypin
Copy link
Contributor Author

oprypin commented Jul 10, 2023

Let's just update sdk/ and rev linter at the same time. Was anyone planning a routine bump of the linter, so perhaps then we could advance it only 1 commit forward when applying this particular change?

If there are some edge cases in their synchronization, I think it's not going to cause much damage, because it's very rare that "./imports" are used. Also people are not forced to only rely on the fixer, it's possible to stop the lint complaint even if they are out of sync.

@oprypin
Copy link
Contributor Author

oprypin commented Jul 11, 2023

OK, linter got bumped to latest! https://dart.googlesource.com/sdk/+/fab30a7a73cd6946fc0cc340a62c10b69740b02c

I think we're in a great position to proceed with this change. Then I'll be able to make an SDK change that will also bump linter to this change.

cc @pq

@srawlins
Copy link
Member

That plan sounds good to me. Landing.

@srawlins srawlins merged commit aed089e into main Jul 12, 2023
6 checks passed
@srawlins srawlins deleted the directives_dot branch July 12, 2023 16:18
@srawlins
Copy link
Member

@oprypin
Copy link
Contributor Author

oprypin commented Jul 12, 2023

Ah CL as well :)
Thanks much!

copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Aug 23, 2023
)

It used to sort imports as follows:

    import './foo1.dart';
    import '../../foo2.dart';
    import '../foo3.dart';
    import 'foo4.dart';

This change corrects it to the following:

    import '../../foo1.dart';
    import '../foo2.dart';
    import './foo3.dart';
    import 'foo4.dart';
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

Successfully merging this pull request may close these issues.

directives_ordering inconsistent with other orderings
4 participants