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

[#237] Modify coloring options #268

Merged
merged 1 commit into from
Jan 23, 2023
Merged

Conversation

aeqz
Copy link
Contributor

@aeqz aeqz commented Jan 19, 2023

Description

Problem: We noticed that output was not being colorized in GitLab CI due to the current implemented guesses for whether showing colors or not.

Solution: On the one hand, we extend the current guesses to also enable coloring by default when the CI env var is set to true. On the other hand, we also add a new flag, color, which avoids these guesses and enables colors.

I have tried in a GitLab CI project and the output is colored now.

Related issue(s)

Fixes #237

✅ Checklist for your Pull Request

Related changes (conditional)

  • Tests

    • If I added new functionality, I added tests covering it.
    • If I fixed a bug, I added a regression test to prevent the bug from
      silently reappearing again.
  • Documentation

    • I checked whether I should update the docs and did so if necessary:
  • Public contracts

    • Any modifications of public contracts comply with the Evolution
      of Public Contracts
      policy.
    • I added an entry to the changelog if my changes are visible to the users
      and
    • provided a migration guide for breaking changes if possible

Stylistic guide (mandatory)

Copy link
Member

@Martoon-00 Martoon-00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good 👍

Leaving one comment, but it's arguable, feel free to ignore it.

[ flag' (Just WithColors) $
long "color" <>
help "Enable ANSI coloring of output. \
\This is enabled by default when `CI` env var is set to true."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good that you included this clarification. However I read it like: "it is enabled if and only if CI env var is set to true" 🤔

Maybe say it differently, like "When CI env var is set to true, this option will be enabled by default". I only swapped the sub-sentences, but to me it seems like avoiding the ambiguity.

I may be wrong about all this though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Martoon-00 I have clarified the option description, and also included some tests regarding the new behavior in CI.

@aeqz aeqz requested a review from Martoon-00 January 20, 2023 13:27
Copy link
Member

@Martoon-00 Martoon-00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat.

Leaving one more comment, but also feel free to ignore it.

tests/golden/check-color/check-color.bats Outdated Show resolved Hide resolved
- anchors:
- color (header I) at src:7:1-7

All repository links are valid.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think this format is just the best ))

Copy link
Contributor Author

@aeqz aeqz Jan 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah 😆, as you may have seen in the commit history, I was trying to store this in a textual representation of the output's binary data, but encountered a lot of problems, mainly:

  • Different behaviour, parameters and output on macOS and linux commands.
  • Different Windows newline character.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, that sounds like a sufferring 😋

@aeqz aeqz force-pushed the aeqz/#237-modify-coloring-options branch from 6614033 to 6e37c94 Compare January 23, 2023 17:15
Problem: We noticed that output was not being colorized in GitLab CI due
to the current implemented guesses for whether showing colors or not.

Solution: On the one hand, we extend the current guesses to also enable
coloring by default when the CI env var is set to true. On the other
hand, we also add a new flag, color, which avoids these guesses and
enables colors.
@aeqz aeqz force-pushed the aeqz/#237-modify-coloring-options branch from 6e37c94 to 29c1ab1 Compare January 23, 2023 17:27
@aeqz aeqz merged commit 747c884 into master Jan 23, 2023
@aeqz aeqz deleted the aeqz/#237-modify-coloring-options branch January 23, 2023 17:39
@int-index int-index mentioned this pull request Dec 27, 2024
5 tasks
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.

[BUG] Xrefcheck lacks coloring when it is desired
2 participants