feat: respect the NO_COLOR environment variable #1360
Conversation
|
@chnn: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/ |
| Heading, | ||
| CallToAction, | ||
| WhoAmIKey, | ||
| } |
There was a problem hiding this comment.
These aren't exactly the same as what was proposed in #309, but are easy to change. In general I leaned towards introducing more variants, since it's a lot easier to combine variants than split them apart (the latter requires understanding the intent of calling code).
There was a problem hiding this comment.
Graph refs were one of the more frequently colored data and may warrant a separate variant from Link.
| std::env::var(key).as_deref(), | ||
| Err(..) | Ok("") | Ok("0") | Ok("false") | Ok("False") | Ok("FALSE") | ||
| ) | ||
| } |
There was a problem hiding this comment.
Do you have a preference for how boolean environment variables should work?
In general I think it's nice to be able to pass a non-empty falsey value to boolean environment variables. If the value of a variable is parameterized in some script somewhere, then setting the value to true or false can be less awkward than either setting or unsetting the variable entirely.
The NO_EMOJI variable doesn't support passing a false value, for what it's worth. I can update either NO_EMOJI or NO_COLOR to be consistent with the other.
There was a problem hiding this comment.
Yeah when we had implemented NO_EMOJI we just check for the presence of the variable, but you're totally right that it's likely easier to set this to true or false in your scripts. Let's go for true/1 (case-insensitive by calling .to_lowercase() on the String) for enabling NO_EMOJI or NO_COLOR and false/0 (also case-insensitive) to disable it.
| @@ -460,13 +457,13 @@ impl RoverOutput { | |||
|
|
|||
| fn print_descriptor(descriptor: impl Display) -> io::Result<()> { | |||
There was a problem hiding this comment.
What does "descriptor" mean in this context? I couldn't figure it out, but the code seems to print a line with header-like formatting.
There was a problem hiding this comment.
Totally - we print a descriptor to stderr for each command output. So like for rover subgraph fetch we let folks know that the output they are looking at is SDL.
Yeah I completely agree here, and I really like how you've laid things out in this PR. If you'd like to further extend this to other output types in another PR I'd be happy to look! It'd be great to cut down on inconsistencies and to have these semantics built in. |
EverlastingBugstopper
added
the
feature 🎉
|
This looks and works great so I'm going to merge this as is and we can extend the design system over time. |
# [0.10.0] - 2022-11-10 > Important: 1 potentially breaking change below, indicated by **❗ BREAKING ❗** ## ❗ BREAKING ❗ - **Fix implementation of `--header` argument - @EverlastingBugstopper, #1369 fixes #1365** This change tightens up usage of the `--header` argument used for `introspect` commands by disallowing previously valid (but undocumented) usage like this: `--header "Header-1: value" "Header-2: value"`. After this change, you _must_ conform to what we have in the documentation, which indicates separate instances of the `--header` argument for each header, like so: `--header "Header-1: value" --header "Header-2: value"`. ## 🚀 Features - **Provide prebuilt binaries for ARM devices - @EverlastingBugstopper, #1356 fixes #582** As of this release, [`rover.apollo.dev`](https://rover.apollo.dev) delivers prebuilt binaries from our GitHub release for ARM devices. Most notably this means that Docker on M1 devices should work out of the box. You should be able to replace any custom builds in your tooling pipeline with a call to the [official curl installer](https://www.apollographql.com/docs/rover/getting-started/#linux--macos-installer). - **Report downstream check task results - @sachindshinde, #1385** When running `rover subgraph check` commands, if the proposed schema would cause downstream failures (i.e. with contracts), those failures are now reported in the check response. - **Faster `rover supergraph compose` - @EverlastingBugstopper, #1392 fixes #992** Rover now resolves all subgraph schemas in parallel when running `rover supergraph compose` on a `supergraph.yaml` file. This should improve the speed to compose large supergraphs significantly. This change also drastically improves error handling by reporting _all_ issues with resolving subgraph schemas (and informing you which schema(s) failed to resolve) rather than exiting on the first failed schema resolution. - **Add `--polling-interval` to `rover dev` - @patrick91, #1377 fixes #1221** You can now set `--polling-interval` when running `rover dev` to change the frequency of introspection poll requests for subgraphs that don't provide the schema from the file system with the `--schema` argument. - **Adds `--skip-update-check` to skip the once-per-day update check - @Tsing, #1396 fixes #1394** Once per day, Rover checks if there is a new version available for update and notifies the user if there is. There is now a flag you can pass to disable this check: `--skip-update-check`. - **Respect the `NO_COLOR` environment variable - @chnn, #1360** `rover` will not use color in any output when executed with the `NO_COLOR` environment variable set to `true`. ## 🛠 Maintenance - **Updates from clap v3 to clap v4 - @EverlatingBugstopper, #1404 fixes #1400** This release updated the command line argument parsing library to major version 4. There should be no noticeable compatibility issues with this update, only lighter binaries. The look and feel of the main `rover --help` output has changed to a neutral color palette along with this change. - **Updates Rust to 1.65.0 - @EverlastingBugstopper, #1399** - **Updates node.js to v18 - @renovate, #1389** - **Updates node dev-dependencies - @renovate, #1204 and zs#1398** - **Remove dependency on the `saucer` crate - @EverlastingBugstopper, #1402** - **Updates `introspector-gadget` to 0.2.0 - @EverlastingBugstopper, #1386** - **Only cache dependencies in CI, not whole `/target` - @EverlastingBugstopper, #1387** - **Use `engine@main` instead of `engine@current` to fetch the API schema - @EverlastingBugstopper, #1368** - **Use `lychee` as a link checker instead of npm - @ptondereau, #1328 fixes #1306** We now use a Rust-based link checker to check the links in the Rover repository instead of a node-based link checker (that was much more flaky). - **Describe latest federation versions in `./latest_plugin_versions.json` - @EverlastingBugstopper, #1363** When you run `rover supergraph compose`, the latest version of composition is automatically downloaded to your machine, these latest version numbers are now stored in `./latest_plugin_versions.json` in the Rover repo. - **Rename `apollo-` headers to `apollographql-` headers - @jsegaran, #1411** - **Update npm to v9 - @renovate, #1412** ## 📚 Documentation - **Update studio algolia key to graphos - @trevorblades, #1384** - **Fix some broken links - @StephenBarlow, #1376** - **Fix a typo in the migration guide instructing the use of `check` instead of `publish` - @EverlastingBugstopper, #1364 fixes #1361**
Closes #309
This commit introduces a new
src/utils/color.rsmodule, which wraps usage of the various.paintmethods fromansi_termin order to support the NO_COLOR environment variable. It also updates existingansi_termusage to route through this utility.I didn't write any tests or docs for this change, mostly because I didn't find any existing precedent for similar behavior. I'm happy to add either tests or docs, just let me know the general pattern you'd like to see.
Supporting
NO_COLORis nice, but to me the bigger change here feels like using semantic names rather than color names for formatting different kinds of output. I think thatroverwould benefit from extending this change into a design system of sorts, so that the different kinds of output are always printed in a consistent and thoughtful way. For example, I noticed that commands are often enclosed in`backticks—a utility could handle this wrapping instead, which ensures that it is consistent everywhere.