Skip to content

Normalize Compact and Expanded reporters #1325

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 1 commit into from
Aug 22, 2020

Conversation

natebosch
Copy link
Member

@natebosch natebosch commented Aug 21, 2020

Working towards #1311

The Compact and Expanded reporters have a lot of code in common,
along with many superficial, and a few not-superficial differences. Some
of the impactful differences look like they may be bugs. Make it easier
to understand both reporters and their differences by reducing the
superficial changes.

The JSON reporter shares less code with the others, but aim for a
consistent field ordering there as well.

  • Avoid using Configuration.current from the compact reporter. This
    was never added in the expanded reporter to avoid a transitive import
    to dart:io. The pattern of being configured at construction is
    already baked in to the ReporterFactory typedef and is a more clear
    pattern than reading from a magic zone scoped variable. Add required
    constructor args for the compact reporter which match those in the
    expanded reporter. Remove the TODO.
  • Use the same pattern for setting the color escape code fields. In the
    future we will likely want to migrate to package:io for these.
  • Shuffle some fields so they are ordered consistently in both
    reporters.
  • Add a default for _lastProgressPassed to avoid making it nullable.
  • Add _printPath and _printPlatform fields in the compact reporter
    rather than read them lazily through the zone scoped configuration.
  • Make the arguments required for the constructors instead of adding
    defaults which are never used.
  • Use 2 slashs for comments.

Working towards #1311

The `Compact` and `Expanded` reporters have a lot of code in common,
along with many superficial, and a few not-superficial differences. Some
of the impactful differences look like they may be bugs. Make it easier
to understand both reporters and their differences by reducing the
superficial changes.

The JSON reporter shares less code with the others, but aim for a
consistent field ordering there as well.

- Avoid using `Configuration.current` from the compact reporter. This
  was never added in the expanded reporter to avoid a transitive import
  to `dart:io`. The pattern of being configured at construction is
  already baked in to the `ReporterFactory` typedef as is a more clear
  pattern than reading from a magic zone scoped variable. Add required
  constructor args for the compact reporter which match those in the
  expanded reporter. Remove the TODO.
- Use the same pattern for setting the color escape code fields. In the
  future we will likely want to migrate to `package:io` for these.
- Shuffle some fields so they are ordered consistently in both
  reporters.
- Add a default for `_lastProgressPassed` to avoid making it nullable.
- Add `_printPath` and `_printPlatform` fields in the compact reporter
  rather than read them lazily through the zone scoped configuration.
- Make the arguments required for the constructors instead of adding
  defaults which are never used.
- Use 2 slashs for comments.
@natebosch
Copy link
Member Author

I did not (or at least not intentionally) change any behavior or fix any bugs here.

@natebosch natebosch requested a review from grouma August 22, 2020 00:09
var _paused = false;

/// The set of all subscriptions to various streams.
final _subscriptions = <StreamSubscription>{};
Copy link
Member

Choose a reason for hiding this comment

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

[nit] move this back if it wasn't touched

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 changed this because all 3 reporters have these same 3 fields directly before the constructor. If you look at a diff between them this minimizes the noise (although admittedly that is less useful in this case because this reporter is more different overall)

@natebosch natebosch merged commit e3008a4 into master Aug 22, 2020
@natebosch natebosch deleted the normalize-reporter-implementations branch August 22, 2020 01:17
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.

3 participants