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

dart format --set-exit-if-changed not really setting the status code #44582

Closed
llucax opened this issue Jan 4, 2021 · 10 comments
Closed

dart format --set-exit-if-changed not really setting the status code #44582

llucax opened this issue Jan 4, 2021 · 10 comments
Assignees
Labels
area-dart-cli Use area-dart-cli for issues related to the 'dart' command like tool. dart-cli-format Issues related to the 'dart format' tool P2 A bug or feature request we're likely to work on

Comments

@llucax
Copy link

llucax commented Jan 4, 2021

I'm not 100% sure the dart cli tool, but I've seen other issues with it reported here, so I guess it it.

When I use dart format --output none --set-exit-if-changed the exit status is not really changed (it used 0):

$ dart --version
Dart SDK version: 2.12.0-133.2.beta (beta) (Tue Dec 15 09:55:09 2020 +0100) on "linux_x64"
$ dart create bug8
Creating /tmp/bug8 using template console-simple...

  .gitignore
  CHANGELOG.md
  README.md
  analysis_options.yaml
  bin/bug8.dart
  pubspec.yaml

Running pub get...                     0.9s
  Resolving dependencies...
  Changed 2 dependencies!

Created project bug8! In order to get started, type:

  cd bug8

$ cd bug8/
$ sed -i '2s/ //g' bin/bug8.dart 
$ cat bin/bug8.dart 
void main(List<String> arguments) {
print('Helloworld!');
}
$ dartfmt --set-exit-if-changed --dry-run .
bin/bug8.dart
$ echo $? # This works as it should
1
$ dart format --set-exit-if-changed --output none .
Changed bin/bug8.dart
Formatted 1 file (1 changed) in 0.12 seconds.
$ echo $? # This doesn't!
0
$ 
@a-siva a-siva added the area-dart-cli Use area-dart-cli for issues related to the 'dart' command like tool. label Jan 5, 2021
@a-siva a-siva added the P2 A bug or feature request we're likely to work on label Jan 5, 2021
@bkonyi bkonyi added the dart-cli-format Issues related to the 'dart format' tool label Jan 5, 2021
@bkonyi bkonyi assigned munificent and unassigned bkonyi Jan 5, 2021
@munificent
Copy link
Member

dart_style should be setting the exit code correctly. I have integration tests in the package to verify. It sets the exit code here using the exitCode setter in dart:io. Is it possible that the CLI integration is overriding that?

@munificent
Copy link
Member

From talking to @devoncarew, it seems likely that this has to do with the integration into Dart CLI, so assigning it back.

@munificent munificent assigned bkonyi and unassigned munificent Feb 20, 2021
@bkonyi
Copy link
Contributor

bkonyi commented Feb 20, 2021

Yeah, we don't use exitCode since we need to pass the value back to the VM through the CLI isolate. Should be an easy enough fix.

@bkonyi
Copy link
Contributor

bkonyi commented Feb 23, 2021

This has been fixed in dart_style and just needs to be rolled into the SDK. Reassigning to @munificent to complete the roll, and I'll add testing for the CLI once this happens.

@bkonyi bkonyi assigned munificent and unassigned bkonyi Feb 23, 2021
@munificent
Copy link
Member

Patch to roll it into the SDK is out for review: https://dart-review.googlesource.com/c/sdk/+/187740

@kuhnroyal
Copy link
Contributor

This was merged in 3f5384b - will there be a 2.12.x patch release with this?

@munificent munificent assigned bkonyi and unassigned munificent Apr 3, 2021
@munificent
Copy link
Member

This is in the SDK now, so bouncing to @bkonyi.

jamesderlin added a commit to google/flutter.widgets that referenced this issue Apr 10, 2021
The `dartfmt` command has been replaced with `dart format`.

We currently can't quite replace `dartfmt` with `dart format` because
the fix for dart-lang/sdk#44582 hasn't
reached the Flutter beta channel yet, so we can't rely on its exit
code.  Additionally, unlike `dartfmt --dry-run`, `dart format
--output=none` prints status to stdout even if nothing's changed, so
we can no longer rely on the output being empty either.

Luckily, `flutter format` does return a proper exit code, so use
that.
jamesderlin added a commit to google/flutter.widgets that referenced this issue Apr 12, 2021
The `dartfmt` command has been replaced with `dart format`.

We currently can't quite replace `dartfmt` with `dart format` because
the fix for dart-lang/sdk#44582 hasn't
reached the Flutter beta channel yet, so we can't rely on its exit
code.  Additionally, unlike `dartfmt --dry-run`, `dart format
--output=none` prints status to stdout even if nothing's changed, so
we can no longer rely on the output being empty either.

Luckily, `flutter format` does return a proper exit code, so use
that.
@vasilich6107
Copy link

Checking in flutter 2.5.1
flutter format --set-exit-if-changed lib - works fine and throws error code 1
dart format --set-exit-if-changed lib - ignores --set-exit-if-changed and always throws error code 0

@munificent
Copy link
Member

I can't repro what you're seeing with the latest Dart SDK:

$ dart format --set-exit-if-changed temp.dart
Formatted temp.dart
Formatted 1 file (1 changed) in 0.15 seconds.
$ echo :$?
:1

I'm not 100% certain that this fix is in Flutter 2.5.1, but I would be surprised if it wasn't. Is it possible that you have an older Dart SDK on your PATH somehow?

@vasilich6107
Copy link

Maybe)
Until it works with flutter format it’s ok for me)
Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-dart-cli Use area-dart-cli for issues related to the 'dart' command like tool. dart-cli-format Issues related to the 'dart format' tool P2 A bug or feature request we're likely to work on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants