Skip to content

Move from dartfmt to dart format #125

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 2 commits into from
Aug 17, 2021
Merged

Move from dartfmt to dart format #125

merged 2 commits into from
Aug 17, 2021

Conversation

natebosch
Copy link
Member

@natebosch natebosch commented Jun 17, 2021

Closes #124

@google-cla google-cla bot added the cla: yes label Jun 17, 2021
@natebosch
Copy link
Member Author

This currently has terrible UX when the code has errors due to https://github.com/dart-lang/sdk/issues/46379

@fishythefish
Copy link
Member

Is this unblocked since dart-lang/dart_style#1035 is closed?

@natebosch
Copy link
Member Author

Now this is blocked by dart-lang/sdk#46636

After this is unblocked we'll need to figure out the rollout plan - given the bugs on existing stable SDKs I don't think we can roll this out universally. We might need to implement some kind of SDK version check. Or we could consider adding a setting which could be more annoying for users, but simpler to implement.

If the `dart` command exists, parse the output of `dart --version` and
for SDK version `2.14` and higher use `dart format`. On older SDKs
fallback to the existing behavior.

There will be some `2.14.0-dev` SDKs which are not compatible with this
check, but they aren't likely be to in use anymore for local
development.
@natebosch natebosch requested a review from sigmundch August 12, 2021 15:53
@natebosch natebosch marked this pull request as ready for review August 12, 2021 15:53
Copy link
Member

@sigmundch sigmundch left a comment

Choose a reason for hiding this comment

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

LGTM

\ '\vDart SDK version: (\d+)\.(\d+)\.\d+.*')
if empty(l:match)
call s:error('Unable to determine dart version')
return []
Copy link
Member

Choose a reason for hiding this comment

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

Rather than return with an error, would it be worth using the fallback in this case?

Copy link
Member

@fishythefish fishythefish Aug 17, 2021

Choose a reason for hiding this comment

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

I think the error might be better? IIRC, it's difficult/tedious to distinguish between stdout and stderr in vim, so diagnostics emitted by dart format/dartfmt get inserted at the top of the file. In particular, if we fail to detect the Dart version and fall back to dartfmt when dart format is expected, the UX will be worse than not formatting at all.

Nate can confirm though - I'm not sure if the linked issues have changed how this works.

Copy link
Member

Choose a reason for hiding this comment

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

got it - error sgtm.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't expect any existing SDK releases to hit this. If the version string format changes in the future SDK releases, the fallback won't be the right behavior.

@natebosch natebosch merged commit 0876462 into master Aug 17, 2021
@natebosch natebosch deleted the dartfmt-deprecated branch August 17, 2021 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update to call new dart tool
3 participants