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

feat: Add option to output colors #2251

Merged
merged 8 commits into from
Mar 20, 2023
Merged

feat: Add option to output colors #2251

merged 8 commits into from
Mar 20, 2023

Conversation

max-sixty
Copy link
Member

@max-sixty max-sixty commented Mar 19, 2023

This moves #1355 forward, and enables a PR to add colors to the book. The main thing to review is whether the proposed structure and naming of the options is reasonable.

On the names — it's not that consistent — we have signature_comment — should this just be colors rather than use_colors? Or does signature_comment suggest it contains an actual comment? I've generally tried to make bools self-describing, but possible an options struct can be more implicit.

Edit: this is now just a color property and .with_color(self, color) method, consistent with something like https://github.com/zesterer/ariadne/blob/ccd465160129d2546d67f7ee78727a4098a64330/src/lib.rs#L81. Possibly we should standardize around something like that


It's slightly different from the proposal in #1355 — it doesn't introduce another layer of options.

  • I think possibly the code has changed slightly since then, since there was a discussion on whether to pass an Options struct to the compile function, which we now already do.
  • And I would vote to lean towards having a flatter, simpler, structure until we have lots of confidence in the eventual structure.

I'm not completely clear how we support colors in prqlc now...


This doesn't implement support in bindings yet.

@max-sixty max-sixty requested a review from aljazerzen March 19, 2023 23:32
@max-sixty
Copy link
Member Author

@aljazerzen given you're quasi-out for two weeks, give this a thumbs-down if you want me to wait — e.g. the options aren't named well — otherwise I'll merge. (better to wait than revert)

@eitsupi
Copy link
Member

eitsupi commented Mar 20, 2023

Could it be listed in the Changelog? (Downstream packages also need to add the new field to Options)

@max-sixty
Copy link
Member Author

Could it be listed in the Changelog? (Downstream packages also need to add the new field to Options)

Definitely, will do

More generally, if there's some check we could add — e.g. anything with a fix or feat requires an edit to the Changelog file, then I'd support that. (But I'm less keen on a separate tool like changie, I find that adds overhead to tiny PRs)

@max-sixty max-sixty merged commit 55ebdb2 into PRQL:main Mar 20, 2023
@max-sixty max-sixty deleted the colors branch March 20, 2023 23:53
max-sixty added a commit to max-sixty/prql that referenced this pull request Mar 20, 2023
Including breaking change note
max-sixty added a commit that referenced this pull request Mar 21, 2023
Including breaking change note
@max-sixty
Copy link
Member Author

I'm not completely clear how we support colors in prqlc now...

FYI this was resolved in #2267

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.

3 participants