Skip to content

Conversation

wileykestner
Copy link
Contributor

@wileykestner wileykestner commented Aug 2, 2023

  • Allow users to select a custom reporter that prints JSON instead of sed commands

  • Opt-in by adding "reporter": "json" to the configuration file

- Allow users to select a custom reporter that prints JSON instead of
  `sed` commands

- Opt-in by adding "reporter": "json" to the configuration file
Copy link
Member

@keith keith left a comment

Choose a reason for hiding this comment

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

looks good! can you ensure newlines at the end of the files

if string == "json" {
self.reporter = JSONReporter()
} else {
self.reporter = nil
Copy link
Member

Choose a reason for hiding this comment

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

can we make this case error for unhandled values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely. Great suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

self.reporter = nil
}
} else {
self.reporter = nil
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self.reporter = nil
self.reporter = SedCommandReporter()

seems fine to default this here so you can eliminate the optionality

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely. 100%.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@wileykestner wileykestner requested a review from keith August 3, 2023 11:19
@keith keith merged commit a606a4e into MobileNativeFoundation:main Aug 3, 2023
@keith
Copy link
Member

keith commented Aug 3, 2023

thanks!

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.

2 participants