prql icon indicating copy to clipboard operation
prql copied to clipboard

Add color option to CLI

Open max-sixty opened this issue 2 years ago • 5 comments

Currently the CLI can't take options such as --color / --no-color. Ideally we'd be able pass these, and they would flow through with an Options struct to the compiler.

A future version could check whether we're in a TTY and default to --no-color if so (it's also possible that clap offers something like this?).

Labeling as Good first issue (which also make good nth issues). As ever, lots of folks will be happy to answer questions!

max-sixty avatar Dec 28 '22 21:12 max-sixty

Hi Max,

I'd like to work on this. My thought of the steps for adding the option are:

  1. Add one enable_color boolean field in the Option struct under the sql/mod.rs.

  2. Add the cli argument enable_color, color, or no-color to the compile command.

  3. Pass the cli argument as an Option field to the compile function

Please correct me if my understanding is wrong.

Shuozeli avatar Dec 29 '22 02:12 Shuozeli

That would be great @Shuozeli !

I think that sounds very reasonable.

I would pass a full Options struct to the compile function (I think that's what you were suggesting). And yes re the --color / --no-color flags.

There's a "smaller" version where we strip out the color at the CLI level, I think that would be easier but less good (but OK!)

@aljazerzen lmk if you have any thoughts!

max-sixty avatar Dec 29 '22 03:12 max-sixty

Hmm, sql does not do any highlighting ATM, so it doesn't need color option (at least for now). But the cli argument would be use to control this param in error formatting. This function is called

The best way to do this would be:

  • add a new Options struct to lib.rs:
    struct Options {
        sql: sql::Options,
        color: bool
    }
    
  • change compile(prql: &str, options: Option<sql::Options>) to this compile(prql: &str, options: Option<Options>)
  • add a impl Default for Options and impl From<sql::Options> for Options for convenience
  • make sure that all crates compile and pass cargo test

aljazerzen avatar Dec 30 '22 11:12 aljazerzen

OK, so we'd have a nested Options struct. That seems very reasonable too.

@Shuozeli does that make sense to you?

max-sixty avatar Dec 30 '22 19:12 max-sixty

Yes, thank you for the clarification. I will be working on this one.

Shuozeli avatar Jan 03 '23 07:01 Shuozeli

@Shuozeli Any progress on this?

vanillajonathan avatar Feb 11 '23 23:02 vanillajonathan