-
Notifications
You must be signed in to change notification settings - Fork 313
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
Allow mash error silencing #488
Conversation
re: hashie#485, we want to be able to allow folks to use warning-less Hashie::Mashes, but at the same time don't want to let gem authors unknowingly disable errors for the consuming projects or other dependencies that may rely on Hashie.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I like this better. Naming-wise I think this is not about warnings, it's about overriding methods that otherwise come from the base class. So maybe we mean subclass
instead of quiet
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that this gives more flexibility around disabling warnings over #487. However, in #485, @nonnenmacher specificly calls out Mash.load
as a culprit for annoyance with the warnings. Personally, I would rather extract Mash.load
into a companion gem and get rid of it, but since it's here we should think about addressing that case.
Do you have any thoughts on how we could incorporate this into Mash.load
? Also, I'd love to hear @nonnenmacher's thoughts on this.
@michaelherold this works with load as well |
🤦♂ Of course it does ... 🤦♂ |
I'm wondering if we should do some sort of caching so we don't generate a new class every time we call Otherwise, I really like this approach! 👍 |
Interesting thought, I may have some time later to look into that |
One argument against is that |
Since we're only memoizing the classes and not the objects instantiated from them, I don't think we need to worry about a non-mutating and mutating method, I'd be open to hearing opposition if I'm not thinking this through all the way 😅 |
I think this is pretty much there AFAIK. A bit of cleanup and we're good to go? |
I think this handles most use cases, faraday_middleware supports passing a custom mash class in which was one of the other concerns. |
Seems like my autoload isn't working in some of the integration tests... do either of you see anything wrong with how I'm autoloading the new extension? |
I think the explicit require is the right move, particularly since we don't use it anywhere else. I'd hedge on removing the autoload, TBH. |
Sound nice ! especially if the |
yep it does, especially as the two mains cases where we have the problems and often are out of reach for code modification (i.e because we can't modify the data) are |
I'm good with this, @michaelherold? |
Go for it! |
Merged. |
re: #485 and #487
@dblock @michaelherold thoughts?