-
Notifications
You must be signed in to change notification settings - Fork 62
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
redirecting stderr at load time is fragile and will hide real issues #70
Comments
So what do you suggest? |
Don't redirect stderr. Or check that its content matches exactly what you expect it to, and only hide it if it does. |
That will not happen. An API to enable / disable method redefinition warnings would help. I might try something like https://github.com/cstjean/ClobberingReload.jl/blob/master/src/scrub_stderr.jl. |
An API that allows you to do what you want to without redefining methods, sure. Redefining methods is something that should be warned about. |
You realize that the whole point of this package right? Edit: Or rather, since there is currently no way of doing what I want without redefining method, I think this is the local maxima right now. |
re: #97 (comment) (deleted comment said "Overwriting methods like this is a bad idea, any time you find yourself doing it it's a sign that the API in question is poorly designed and really needs to be improved before 1.0 so these awful hacks can be temporary. Hiding actual problems from your users at load time to suppress legitimate warnings is a really bad tradeoff. You should not have to overwrite methods - when you do, that's a problem that needs fixing, and not by pretending it doesn't exist.") I apologize for the harsh language, but I'm specifically referring only to https://github.com/KristofferC/OhMyREPL.jl/blob/v0.2.7/src/OhMyREPL.jl#L126-L130, and the method overwrites that make it necessary. Which you've acknowledged elsewhere is a hack that you'd rather not have to do. It's the monkey patching that shouldn't be necessary long term. We have a ways to go still, but the fact that you have to monkey patch and overwrite them instead of properly extending them means the Base API's still need work. |
suppressing warnings (which are there because this package does things most code shouldn't) is going to also silently swallow any real errors or deprecations
The text was updated successfully, but these errors were encountered: