Skip to content

Add import sorting to format tool #382

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

Merged
merged 23 commits into from
Jul 26, 2022
Merged

Add import sorting to format tool #382

merged 23 commits into from
Jul 26, 2022

Conversation

regenvanwalbeek-wf
Copy link
Contributor

Note: A backpatch for this change exists here for consumers who can't upgrade to analyzer 1.0.0

Having to manually sort imports is tedious. It would be better if we could just run the formatter to receive sorted imports.

I added an organizeImports flag to the FormatTool. When set to true, it will also run the file through an import sorter.

The import sorter tries to comply with https://dart.dev/tools/linter-rules#directives_ordering

- Reviewer note: This is copied from dart_dev_workiva for easy reviewing.
- This should only contain import changes
- This will allow us to execute multiple processes as
part of a `ddev format` command
- I would rather follow Effective Dart guidelines. directives_ordering
no longer separates own package from other package. See
https://dart.dev/guides/language/effective-dart/style#ordering
- prefer_relative_imports seems like a better alternative for packages
that wish to separate own package imports
https://dart.dev/guides/language/effective-dart/usage#prefer-relative-import-paths
@regenvanwalbeek-wf regenvanwalbeek-wf changed the title Import sort Add import sorting to format tool Jul 14, 2022
@aviary3-wk
Copy link

Security Insights

No security relevant content was detected by automated scans.

Action Items

  • Review PR for security impact; comment "security review required" if needed or unsure
  • Verify aviary.yaml coverage of security relevant code

Questions or Comments? Reach out on Slack: #support-infosec.

@regenvanwalbeek-wf regenvanwalbeek-wf marked this pull request as ready for review July 18, 2022 14:20
@regenvanwalbeek-wf
Copy link
Contributor Author

@evanweible-wf The functional tests I added appear to be failing on stable (pass on 2.13). They're passing for me locally running on stable. Do you see anything immediately obvious?

evanweible-wf
evanweible-wf previously approved these changes Jul 25, 2022
@autobot-wf
Copy link

Skynet test results failed initially for this build but were approved by evan.weible
https://wf-skynet-hrd.appspot.com/apps/test/smithy/3780815/1
Approval message: CI failures on beta/dev are known due to removal of dartanalyzer

@evanweible-wf
Copy link
Contributor

QA +1

  • CI passes on 2.13.4 and stable
  • Failures on beta/dev are known due to removal of dartanalyzer, but are failing because the default behavior of ddev analyze is to use dartanalyzer and we can't easily change that without raising our minimum SDK, so we'll do that later.

@Workiva/release-management-p

import 'package:analyzer/dart/ast/ast.dart';
import 'package:analyzer/dart/ast/token.dart';

/// A representation of an namespace directive.
Copy link
Member

Choose a reason for hiding this comment

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

#super_minor: there are a few places like this in this file; I assume it's because this used to read "an import directive". 🤷

Suggested change
/// A representation of an namespace directive.
/// A representation of a namespace directive.

Copy link
Contributor

@rmconsole-wf rmconsole-wf left a comment

Choose a reason for hiding this comment

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

+1 from RM

@rm-astro-wf rm-astro-wf merged commit bef3239 into master Jul 26, 2022
@rm-astro-wf rm-astro-wf deleted the import_sort branch July 26, 2022 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants