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

Fixes YAML serialization/deserialization for Faraday::Utils::Headers #705

Merged
merged 1 commit into from
Jul 12, 2017

Conversation

iMacTia
Copy link
Member

@iMacTia iMacTia commented Jul 11, 2017

Fixes #690

@iMacTia
Copy link
Member Author

iMacTia commented Jul 11, 2017

@olleolleolle @seanbjornsson @AvaelKross
I'm really sorry for the delay on this, but I finally got some time to spend investigating the issue.

Issue with base functionality (before @AvaelKross / @olleolleolle fix):
YAML.load calls #allocate on the object class (in this case Faraday::Utils::Headers) which skips the call to #initialize. In our case, this was causing @names to be uninitialised ("undefined method '[]' for nil:NilClass")

Issue with encode_with/decode_with (after @AvaelKross / @olleolleolle fix):
encode_with and decode_with are not extending the base behaviour of #to_yaml, but rather overriding it. This is causing all the key/value pairs to get lost in the process

Final solution
Unfortunately I had to revert @AvaelKross / @olleolleolle changes. Instead, I've overridden #allocate in Faraday::Utils::Headers initialising the @names instance variable.

Your input on the above will be much appreciated before I merge this in 😄
Moreover, tests have been upgraded with suggestions from @seanbjornsson, but if you have any personal project where you can test this special branch it would be even better.

@iMacTia iMacTia added this to the v0.12.2 milestone Jul 11, 2017
@olleolleolle
Copy link
Member

@iMacTia I learned about #allocate just yesterday and now, now, you use it in a crucial place. THANK YOU, and wow serendipity!

@iMacTia
Copy link
Member Author

iMacTia commented Jul 11, 2017

Ahahah call them "coincidences" if you want 😄
Thanks for the feedback and I'm glad you liked the solution 😃

@iMacTia iMacTia merged commit 94097a7 into master Jul 12, 2017
@iMacTia iMacTia deleted the fix/#690 branch July 12, 2017 15:57
@seanbjornsson
Copy link

Fantastic @iMacTia, thank you for coming back around to this! Sorry I couldn't respond sooner.

The spec additions, however simple, capture my use case perfectly.
I'll see about updating my project, and ripping out my monkey patch fix.
Thanks again.

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.

3 participants