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

Allow mash error silencing #488

Merged
merged 11 commits into from
Oct 14, 2019
Merged

Conversation

BobbyMcWho
Copy link
Collaborator

@BobbyMcWho BobbyMcWho commented Oct 6, 2019

re: #485 and #487
@dblock @michaelherold thoughts?

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.
spec/hashie/mash_spec.rb Outdated Show resolved Hide resolved
Copy link
Member

@dblock dblock left a 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?

lib/hashie/mash.rb Outdated Show resolved Hide resolved
Copy link
Member

@michaelherold michaelherold left a 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.

lib/hashie/mash.rb Outdated Show resolved Hide resolved
lib/hashie/mash.rb Show resolved Hide resolved
@BobbyMcWho
Copy link
Collaborator Author

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.

@michaelherold this works with load as well Hashie::Mash.quiet.load(path, parser: parser)

@michaelherold
Copy link
Member

🤦‍♂ Of course it does ... 🤦‍♂

@michaelherold
Copy link
Member

I'm wondering if we should do some sort of caching so we don't generate a new class every time we call .quiet. Maybe a lookup table with keys based on the method keys you send to the method?

Otherwise, I really like this approach! 👍

@BobbyMcWho
Copy link
Collaborator Author

Interesting thought, I may have some time later to look into that

@dblock
Copy link
Member

dblock commented Oct 7, 2019

I'm wondering if we should do some sort of caching so we don't generate a new class every time we call .quiet. Maybe a lookup table with keys based on the method keys you send to the method?

One argument against is that quiet would work like merge, always generating a new thing. Maybe we can also have quiet! that mutates the existing class?

@BobbyMcWho
Copy link
Collaborator Author

One argument against is that quiet would work like merge, always generating a new thing. Maybe we can also have quiet! that mutates the existing class?

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 😅

CHANGELOG.md Outdated Show resolved Hide resolved
@dblock
Copy link
Member

dblock commented Oct 10, 2019

I think this is pretty much there AFAIK. A bit of cleanup and we're good to go?

@BobbyMcWho
Copy link
Collaborator Author

I think this handles most use cases, faraday_middleware supports passing a custom mash class in which was one of the other concerns.

@BobbyMcWho BobbyMcWho marked this pull request as ready for review October 10, 2019 14:16
@BobbyMcWho
Copy link
Collaborator Author

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?

@michaelherold
Copy link
Member

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.

@nonnenmacher
Copy link

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.

@michaelherold this works with load as well Hashie::Mash.quiet.load(path, parser: parser)

Sound nice !
and would do perfectly I guess

especially if the parser is an argument, as one of the problem of Safe_yaml and underlying implementation (e.g psych versus sick) don't allow passing options from the parse call).

@nonnenmacher
Copy link

I think this handles most use cases, faraday_middleware supports passing a custom mash class in which was one of the other concerns.

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
-- loading configurations that have defined/not changeable names
-- REST responses that contains attributes out of reach (e.g this zip thing)

@dblock
Copy link
Member

dblock commented Oct 12, 2019

I'm good with this, @michaelherold?

@michaelherold
Copy link
Member

Go for it!

@dblock dblock merged commit 1a30427 into hashie:master Oct 14, 2019
@dblock
Copy link
Member

dblock commented Oct 14, 2019

Merged.

@messiias messiias mentioned this pull request Sep 13, 2021
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.

4 participants