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

Fix console reader issues #219

Merged
merged 3 commits into from Apr 6, 2022
Merged

Conversation

djspiewak
Copy link
Contributor

Three issues! First, loads of commands can crash under certain circumstances, such as when cloned without tags. Wrapping everything in Try should fix this. Second, grepping git status is very much the wrong way to look for uncommitted changes. This is why git status -s exists. Third, the logger is very chatty, so I made it configurable and dropped the default level to Debug.

This is blocking typelevel/cats-effect#2942

@lightbend-cla-validator
Copy link

Hi @djspiewak,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement:

https://www.lightbend.com/contribute/cla

Copy link

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

Not an independent review since I'm a CE maintainer, but changes here are at the very least reasonable and definitely applicable to our use if not everyone else's. The default log level is of course a matter of opinion :)

Politely pinging @hughsimpson and @blast-hardcheese for additional review (if you are willing and able! :) who were involved with the original PR adding this feature in #168.

Copy link

@blast-hardcheese blast-hardcheese left a comment

Choose a reason for hiding this comment

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

These are sensible improvements on a known hacky workaround. Other than the toOption's making it harder to track down issues where things don't work well, this looks good to me. (In the error cases, do we still get info to stderr?)

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

5 participants