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

Regard --color / --no-color #24

Merged
merged 1 commit into from Sep 8, 2023
Merged

Regard --color / --no-color #24

merged 1 commit into from Sep 8, 2023

Conversation

sol
Copy link
Member

@sol sol commented Feb 26, 2023

@sol sol marked this pull request as ready for review February 26, 2023 05:46
@sol sol marked this pull request as draft February 26, 2023 05:46
@sol sol marked this pull request as ready for review May 12, 2023 01:39
@sol
Copy link
Member Author

sol commented May 12, 2023

@parsonsmatt this is ready to go.

Copy link
Collaborator

@parsonsmatt parsonsmatt left a comment

Choose a reason for hiding this comment

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

Just so I understand correctly - this skips the hedgehog color check and instead uses the hspec --color and --no-color options, where --no-color strips out the ANSI codes.

Hedgehog's detectColor function does some other logic, though, like HEDGEHOG_COLOR env var check and ensuring the logged in user isn't mth lol.

I feel like we can retain the functionality here with a much broader range of hspec versions by detecting color as-is, and then use CPP to use either Reason or ColorizedReason.

I'm hesitant to force consumers to upgrade to the latest hspec versions only for this

@sol
Copy link
Member Author

sol commented Jul 18, 2023

Just so I understand correctly - this skips the hedgehog color check and instead uses the hspec --color and --no-color options, where --no-color strips out the ANSI codes.

Correct.

Hedgehog's detectColor function does some other logic, though, like HEDGEHOG_COLOR env var check and ensuring the logged in user isn't mth lol.

I feel like we can retain the functionality here with a much broader range of hspec versions by detecting color as-is, and then use CPP to use either Reason or ColorizedReason.

If I understand you correctly, then no, I don't think that would solve my use case. If I specify --color and e.g. redirect stdout then detectColor will return False. As a consequence I will end up without colors, even though I requested them.

I'm hesitant to force consumers to upgrade to the latest hspec versions only for this

Do you think there is any tangible benefit from doing this, that is relevant enough to offsets the additional complexity? Note that existing users can continue to use 0.0.1.2 with older versions of hspec.

@sol
Copy link
Member Author

sol commented Sep 7, 2023

@parsonsmatt any chance to get this merged sometime soon? Maintaining and using my private fork, which is not on Hackage, will eventually be too much overhead for me.

@parsonsmatt
Copy link
Collaborator

Sure - please make a version bump and changelog entry and I'll have it released asap. Thanks!

@sol
Copy link
Member Author

sol commented Sep 8, 2023

Sure - please make a version bump and changelog entry and I'll have it released asap. Thanks!

Done as a separate PR (#27) to not unnecessarily cause conflicts.

@parsonsmatt parsonsmatt merged commit c93b586 into hspec:master Sep 8, 2023
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.

None yet

2 participants