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 ability to capture log prints outside of package #51

Closed
wants to merge 6 commits into from

Conversation

requaos
Copy link

@requaos requaos commented Nov 15, 2018

I use this as a library and want to be able to send all output to my own logger, so I propose this change

@dhowden
Copy link
Member

dhowden commented Nov 15, 2018

Based on the comments in the code, I think those existing log statements were marked for removal and instead the errors would be returned (making it much easier for you to handle them in your application).

In general, I think this would be a better approach - as it will also make it easier for callers to handle the errors, rather than having to retrospectively read them in a log.

@requaos
Copy link
Author

requaos commented Nov 16, 2018

I agree, however, the need for this pr arose from non-fatal warning errors being printed directly to stderr without any formatting. In the case of wxSummary, there is a high probability that a segmentation fault will occur, but this has little bearing on the final text output and should be considered as a warning which should not stop the flow of execution.

I can touch up this PR to make the flow more idiomatic go, but in the immediate sense I was not trying to alter the original flow, just provide an output modifier

@requaos
Copy link
Author

requaos commented Nov 17, 2018

@dhowden Please review the error handling pattern in this last commit to doc.go, as I would like your feedback before proceeding.

Copy link
Member

@dhowden dhowden left a comment

Choose a reason for hiding this comment

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

I can see where you're going with this, but to convert the whole library will be a huge undertaking, not least because we need to ensure that we trap+wrap+unwrap and then classify all the errors. This will be very tricky to get right.

Let me have a chat here and see what we can come up with. In the short term, it might be that setting a logger is the easiest path.

doc.go Outdated Show resolved Hide resolved
@jonathaningram
Copy link
Member

#153 removed internal log messages in a v2. We'll probably be able to pass around an slog.Logger in some form in a future PR to allow for logging to be reintroduced. Closing this PR for now.

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

3 participants