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

chore(cli): change name of --output option to --format #1413

Merged
merged 31 commits into from
Jan 19, 2023

Conversation

EverlastingBugstopper
Copy link
Contributor

fixes #1212

@gocamille gocamille force-pushed the camille/update-format-option branch from ba8e1ce to 0c0824d Compare November 24, 2022 15:47
@gocamille gocamille marked this pull request as ready for review November 28, 2022 13:13
@gocamille gocamille requested a review from lrlna as a code owner November 28, 2022 13:13
@gocamille
Copy link
Contributor

gocamille commented Nov 28, 2022

Description

This PR adds a new option, --format, to allow users to define the format type (either plain or json) for stdout messages. Fixes #1212.

Motivation and Context

According to the Command Line Interface Guidelines, use of the --output (--o for short) option in a CLI should be restricted to defining a file output. Rover’s current implementation of --output is for defining the format type for messages to stdout. This PR updates Rover to be in line with the Guidelines by:

  • Adding a --format option type that can take either plain or json as arguments to handle formatting messages to stdout.
  • Adding a deprecation warning when a user uses the --output option, with a suggestion to use the new --format option.
  • Throwing an error if a user attempts to enter both --output and --format in the same command.
  • Add another type to the --output option of Utf8PathBuf to handle future arguments to this option when it is refactored for defining file outputs.

Other considerations:

  • The get_stdout() function returns an optional String result because some RoverOutput types do not return anything to stdout. Ideally all output types return some signal to the user. Issues #1436, #1437, and #1438 address this.

  • We now provide messaging when users decide to output to a file.

@gocamille gocamille force-pushed the camille/update-format-option branch from b92d9b7 to 2bb75f9 Compare November 28, 2022 14:18
@EverlastingBugstopper
Copy link
Contributor Author

There's this issue as well since I don't think rover config whoami prints to stdout: #1380

Copy link
Contributor Author

@EverlastingBugstopper EverlastingBugstopper left a comment

Choose a reason for hiding this comment

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

left a note on refactoring, but also at a higher level - specifying --output file.txt doesn't ever write anything to that file! we should check to see what the output destination is before printing - if it's stdout, print it, if it's a file, create and write to the file (using rover_std::Fs). also, there are a few lints that clippy picked up in CI that we'll need to fix up.

This is great work, keep it up!

@gocamille gocamille force-pushed the camille/update-format-option branch from 9a66063 to 1625623 Compare November 30, 2022 14:09
@gocamille gocamille force-pushed the camille/update-format-option branch from d721923 to ee07e6e Compare January 9, 2023 00:49
Copy link
Contributor Author

@EverlastingBugstopper EverlastingBugstopper left a comment

Choose a reason for hiding this comment

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

sssooo close!! great work on this, so excited to see it land 😸

}
(None, Some(OutputType::LegacyOutputType(_))) => {
let warn_prefix = Style::WarningPrefix.paint("WARN:");
eprintln!("{} The argument '--output' will soon be deprecated. Please use the '--format' argument to specify the output type.", warn_prefix);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
eprintln!("{} The argument '--output' will soon be deprecated. Please use the '--format' argument to specify the output type.", warn_prefix);
eprintln!("{} The argument for specifying the format of Rover's output has been renamed from '--output' to '--format'. Please use the '--format' argument to specify the output format instead of '--output'. Future versions of Rover may be incompatible with this usage.", warn_prefix);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @StephenBarlow on this wording

@@ -76,15 +76,17 @@ If Rover log messages are unhelpful or unclear, please leave us feedback in an

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @StephenBarlow -- let us know if this copy needs to be refined!

@EverlastingBugstopper EverlastingBugstopper added the feature 🎉 new commands, flags, functionality, and improved error messages label Jan 18, 2023
@EverlastingBugstopper EverlastingBugstopper added this to the vNext milestone Jan 18, 2023
@EverlastingBugstopper
Copy link
Contributor Author

Unfortunately I ran into a bug with this when used in combination with the --watch flag on rover graph introspect and rover subgraph introspect - the watcher should be writing files out to disk on changes but it is not. This isn't something we discussed and so I'm happy to take on this fix.

@EverlastingBugstopper EverlastingBugstopper merged commit c2ad28b into main Jan 19, 2023
@EverlastingBugstopper EverlastingBugstopper deleted the camille/update-format-option branch January 19, 2023 20:02
EverlastingBugstopper added a commit that referenced this pull request Jan 24, 2023
# [0.11.0] - 2023-01-24

## 🚀 Features

- **Manage contract configuration - @sachindshinde, #1475 fixes #1421**
 
Rover now includes two commands for creating, modifying, and reading
contracts: `rover contract publish` and `rover contract describe`.
Further documentation can be found
[here](https://www.apollographql.com/docs/rover/commands/contracts).

- **Easier file output with new `--output` argument - @gocamille, #1413
fixes #1212**

The `--output` argument has long been used to configure the format of
Rover's output. i.e. `--output json` configures Rover to print its
output as a JSON object. This argument has been _renamed_ to `--format`
while maintaining backwards compatibility. `--format json` should be
used to configure the format of Rover's output, and the `--output`
argument allows you to specify a file to print the output to. Adding
`--output schema.graphql` to a `rover subgraph fetch` command will
output your schema to a file. `--output data.json --format json` will
output the command data to `data.json`. `--output json` will still work
by itself but will now print a warning, and `--output [json|plain]` does
not work with `--format [json|plain]`. Further documentation can be
found
[here](https://www.apollographql.com/docs/rover/conventions#output-to-a-file).

- **Adds `--router-config` to `rover dev` - @EverlastingBugstopper,
#1446 fixes #1373, #1345, and #1468**

The new `--router-config` argument for `rover dev` allows you to pass a
[router configuration file]() on startup that can configure settings
such as header propagaion and CORS policies. Further documentation can
be found
[here](https://www.apollographql.com/docs/rover/commands/dev/#advanced-configuration).

- **Auto-update router versions in `rover dev` - @EverlastingBugstopper,
#1432**

`rover dev` will automatically use the version of the router specified
in [this plugin
file](https://github.com/apollographql/rover/blob/main/latest_plugin_versions.json)
instead of a hard coded version.

## 🛠 Maintenance

- **Better error and help text for ELv2 license argument - @DoumanAsh,
#1456 fixes #1455**

The help text for the `--elv2-license` argument now includes the
expected value for the argument, in addition to the error message
returned when the argument is needed but not passed.

- **Updates the Ariadne template URL - @patrick91, #1439**

- **Updates `./examples/supergraph-demo` to `@apollo/server` v4, and
removes `./examples/dev` - @EverlastingBugsttopper, #1442 fixes #1440
and #1441**

- **Updates dependencies - @EverlastingBugstopper, #1481, #1450**

  `apollo-parser` 0.3 -> 0.4
  `base64` 0.13 -> 0.21
  `git2` 0.15 -> 0.16
  `graphql_client` 0.11.0 -> 0.12
  `serial_test` 0.9 -> 1.0
  `os_info` 3.4 -> 3.5
  `os_type` 2.4 -> 2.6
  `termcolor` 1.1 -> 1.2
  `tokio` 1.21 -> 1.24

## 📚 Documentation

- **Fixes a link to schema check example - @MayCXC, #1431**
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature 🎉 new commands, flags, functionality, and improved error messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: modify --output to output to a file instead of determine the output type
2 participants