-
Notifications
You must be signed in to change notification settings - Fork 42
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
fix no-colors for help/version #878
Conversation
I'm OK generally with lenses when they make things more pleasant; we use them in quite a few places. I do tend to avoid TH unless it is a big win. I'll leave it up to your judgment whether this qualifies. |
I just pushed a commit trying to use I like the approach a lot:
|
This seems fine to me. I guess it's a little annoying that we can't twiddle color options via the configuration file (if I've understood right), but maybe that's not too bad. |
This seems very reasonable overall. As a note, to fix #887 I'll need to make some adjacent changes, so if you think you'll land this soonish I'd rather wait to avoid conflicts. |
I can try and put out the final touches on this one this afternoon! |
b5dc2b7
to
495512b
Compare
Fixed and rebased (if you had already looked at this, you can only check the last two commits). |
Opening this as a draft pull request because the current design makes this very awkward.
Paging @robdockins and @kquick who might have ideas/concerns.
Problem: The
--no-colors
flag I added does not remove colors for--help
and--version
. This was known, it was just much harder to support it for those than the rest.Reason: Those options are parsed "early", before Crux parses the rest of the options, to terminate the program early. As a result, they are executed before the
--no-colors
flag is even read.Solution: One can make the
--no-colors
flag be parsed early, so that the help and version commands can use colors appropriately.However, this creates a cascade of other annoyances. First, to pass the options to the logger, we need to either:
CruxOptions ColorOptions OtherOptions
,CruxOptions
all those options that are related to output (that have sensible default values), so that we can pass thoseOutputOptions
to the logger.Of course, any attempt to de-flatten the structure of
CruxOptions
makes it somewhat annoying to perform nested updates without lenses. I'm not sure what's everyone's feeling on the use of lenses, and the use of Template Haskell to generate them. In this draft, I'm using lenses, but rather than bringingTemplateHaskell
, which can make things slower, I just hand-write one lens.This brings up the question of whether we should lensify the whole thing.
Because I chose a structure:
there is still a very awkward setting of
defaultColorOptions
incruxOptions :: Config CruxOptions
, because it has to return aCruxOptions
but cannot know the color options.If we were to go an alternative route of:
we would get rid of this awkwardness, but manipulating this without lenses would also be unpleasant.
TL;DR:
There are four set of options:
--help
and--version
while respecting--no-colors
, we need to parse (1.) and (2.) early.--help
and--version
runs.