-
-
Notifications
You must be signed in to change notification settings - Fork 22
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
Resolves [#22] Add CLI options to handle colouring #93
Resolves [#22] Add CLI options to handle colouring #93
Conversation
Hey @marcellourbani 👋🏻 Sorry for the delay in review. This PR is a lot of work and will take a while for me to review 😅 |
No worries, I'm on holiday with no laptop anyway
…On Wed, 19 Oct 2022, 13:26 Dmitrii Kovanikov, ***@***.***> wrote:
Hey @marcellourbani <https://github.com/marcellourbani> 👋🏻
Sorry for the delay in review. This PR is a lot of work and will take a
while for me to review 😅
—
Reply to this email directly, view it on GitHub
<#93 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AASW6HMAU742OZRUMV3MSCLWD7LIDANCNFSM6AAAAAARGMGTNQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @marcellourbani 👋🏻
I finally have the capacity and motivation to do some work on Iris so I was able to review your PR 🙂 Huge work, thanks a lot for that! 👏🏻
I left some comments with potential improvements to this issue. Let me know if you're still interested in continuing working on this issue 🙂
Otherwise, I can take it over from here and finish, no problemo 👌🏻
Also, CI stopped working a while ago, so you need to rebase on top of the latest main
branch to make it work again in this PR.
Thanks @chshersh .I'm quite interested and will look into all of these, on my own time. Probably later this week |
Sorted all except the ColourOption constructors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks a lot!
I'm currently focusing on merging all existing PRs and I'll prepare a new Iris release after that 🙂
Resolves #22
Iris.Cli.Colour
always
/auto
/never
optionsIris.Colour.Detect
with the implementation of the colouring detection logic described in the beginning of this issuestdout
andstderr
. In other words, env variables and CLI disable / enableI had to change expectations of some tests in cliSpecParserConflicts to account for the new options, I wonde if they can be made more robust
Also, I guess I should also have added tests for conflicts of --no-colour and friends. Didn't feel like doing that today, and I'll be on holiday next week, so I thought was good enough for now
Additional tasks