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

Add logs over commands #375

Merged
merged 5 commits into from
Oct 23, 2022
Merged

Add logs over commands #375

merged 5 commits into from
Oct 23, 2022

Conversation

blackheaven
Copy link
Contributor

I'm tackling #372 with a twist:

  • Long running command is at INFO
  • Commands per se are at DEBUG level

I think it gives enough feedback for day-to-day use, but allows to have a more verbose if needed.

Regarding design, it introduces a lot of burden, I wonder if I'd better to integrate it in Cradle.

WDYT?

@fendor
Copy link
Collaborator

fendor commented Oct 20, 2022

Sorry for the long delay! I just kept putting off the work to review 😅

@fendor
Copy link
Collaborator

fendor commented Oct 20, 2022

The proposed changes are breaking changes, while there is nothing wrong with breaking changes in hie-bios, I would like to keep the breaking change as minimal as possible.

It seems like you do a lot of parameter passing just to add it to runGhcCmd. How about we add it as a paramater to runGhcCmd, because otherwise it feels redundant and weird to pass a log function to runCradle and pass the log action for runGhcCmd via the cradle generation. I think that'd make it more consistent. That, or making runCradle to also use the log action passed in to the Cradle creation function.

Otherwise, lgtm, thank you for your contribution!

@blackheaven
Copy link
Contributor Author

No worries, thanks for your feedback, I have pushed a new version integrating them.

Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@fendor fendor merged commit 45a2711 into haskell:master Oct 23, 2022
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