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

Add color option to CLI #1355

Closed
max-sixty opened this issue Dec 28, 2022 · 6 comments · Fixed by #2267
Closed

Add color option to CLI #1355

max-sixty opened this issue Dec 28, 2022 · 6 comments · Fixed by #2267
Labels
compiler friendliness good first issue Tasks suited for learning the compiler codebase

Comments

@max-sixty
Copy link
Member

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 max-sixty added friendliness good first issue Tasks suited for learning the compiler codebase labels Dec 28, 2022
@Shuozeli
Copy link
Contributor

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.

@max-sixty
Copy link
Member Author

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!

@aljazerzen
Copy link
Member

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

@max-sixty
Copy link
Member Author

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

@Shuozeli does that make sense to you?

@Shuozeli
Copy link
Contributor

Shuozeli commented Jan 3, 2023

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

@vanillajonathan
Copy link
Collaborator

@Shuozeli Any progress on this?

max-sixty added a commit to max-sixty/prql that referenced this issue Mar 21, 2023
This adds `--color` & `--include-signature-comment` options to the `prqlc compile` command.

Closes PRQL#1355. I think we did a not-great job at defining the minimum requirements there, to the extent that this is a simpler construction that leans more heavily on external libraries -- and is both simpler and has better functionality as a result. Even without the external libraries, I think we could have suggested a flatter structure to the `Options' struct.

(Or maybe we'll decide this construction is too simple / leans too much on external libraries).
max-sixty added a commit that referenced this issue Mar 21, 2023
* feat(prqlc): Add color & signature_comment options

This adds `--color` & `--include-signature-comment` options to the `prqlc compile` command.

Closes #1355. I think we did a not-great job at defining the minimum requirements there, to the extent that this is a simpler construction that leans more heavily on external libraries -- and is both simpler and has better functionality as a result. Even without the external libraries, I think we could have suggested a flatter structure to the `Options' struct.

(Or maybe we'll decide this construction is too simple / leans too much on external libraries).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler friendliness good first issue Tasks suited for learning the compiler codebase
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants