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

Don't write log messages to stdout #3159

Merged
merged 7 commits into from
Nov 11, 2024
Merged

Don't write log messages to stdout #3159

merged 7 commits into from
Nov 11, 2024

Conversation

lukaszcz
Copy link
Collaborator

@lukaszcz lukaszcz commented Nov 7, 2024

The "Cloning (...)" message was written to stdout which messed up markdown generation.

@lukaszcz lukaszcz added this to the 0.6.8 milestone Nov 7, 2024
@lukaszcz lukaszcz requested a review from janmasrovira November 7, 2024 12:54
@lukaszcz lukaszcz self-assigned this Nov 7, 2024
@lukaszcz lukaszcz requested a review from paulcadman November 7, 2024 12:54
Copy link
Collaborator

@janmasrovira janmasrovira left a comment

Choose a reason for hiding this comment

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

I don't think we should suppress git messages

@paulcadman
Copy link
Collaborator

I think we should have logs for when things are being cloned in the normal use-case. Perhaps we could configure the output based on the current log level instead to cover use cases where you don't want them printed?

@lukaszcz
Copy link
Collaborator Author

lukaszcz commented Nov 7, 2024

I can turn back git messages. They go to stderr, so it's not really a problem. The main thing is redirecting the log function to use stderr instead of stdout.

The problem for markdown generation is that the log goes to stdout. Do you have a reason to write it to stdout?

@janmasrovira
Copy link
Collaborator

janmasrovira commented Nov 7, 2024

Ideally we would redirect the git messages to the Logger effect, probably on log level info or progress

@lukaszcz
Copy link
Collaborator Author

lukaszcz commented Nov 7, 2024

But do we need the logger effect to then write it to stdout? If I just change this to stderr, this will already solve the markdown generation problem.

@janmasrovira
Copy link
Collaborator

I mentioned the Logger effect, which prints to stderr, so that it integrates well with the existing cli flags

@lukaszcz
Copy link
Collaborator Author

lukaszcz commented Nov 7, 2024

Juvix.Data.Effect.Log writes to stdout, unless there is some other logger effect. Actually, I didn't change it, I only changed to stderr for the buffering

@lukaszcz
Copy link
Collaborator Author

lukaszcz commented Nov 7, 2024

Changed to stderr now

@lukaszcz
Copy link
Collaborator Author

lukaszcz commented Nov 7, 2024

Ah, okay, there is also Juvix.Data.Logger. Maybe we should remove Juvix.Data.Effect.Log then?

@paulcadman paulcadman merged commit 76af212 into main Nov 11, 2024
4 checks passed
@paulcadman paulcadman deleted the clone-be-quiet branch November 11, 2024 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants