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

Deserialize BigDecimal correctly from YAML using Psych #589

Closed
wants to merge 6 commits into from

Conversation

jarmo
Copy link

@jarmo jarmo commented Oct 8, 2013

Fixes #588.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) when pulling 5f1c475 on jarmo:master into 02c7032 on collectiveidea:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) when pulling ed14ca6 on jarmo:master into 02c7032 on collectiveidea:master.

@jarmo
Copy link
Author

jarmo commented Oct 8, 2013

For some reason it fails for Ruby 1.9.2 - maybe that version of Ruby does not have the code for deserializing BigDecimal. I don't have any 1.9.2 lying aroud to check that.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) when pulling e8db786 on jarmo:master into 02c7032 on collectiveidea:master.

@ledermann
Copy link

+1 for merge

@jarmo
Copy link
Author

jarmo commented May 2, 2014

Still not merged? Sigh.

@brupm
Copy link

brupm commented Jul 3, 2014

👍

@albus522
Copy link
Member

albus522 commented Jul 3, 2014

This has several unrelated changes and don't use define_method. Use alias_method like the rest of the project.

@jarmo
Copy link
Author

jarmo commented Jul 4, 2014

@albus522 What unrelated changes do you have in mind? If you mean the changes related with Windows platform then i don't see them as unrelated changes per se, because they were needed for me to make that patch since i'm developing on that platform.

Also, define_method in this case is more correct/clean opposed to alias_method since it does not pollute the namespace with unneeded methods. And if you say that alias_method is used for other similar problem solutions then that does not mean it is best solution. alias_method should be used for aliasing methods and not for "overrides".

@albus522
Copy link
Member

albus522 commented Jul 4, 2014

They are unrelated to deserializing BigDecimal and again please use alias_method

@noelrappin
Copy link

Okay, what's stopping this one from being merged?

@albus522
Copy link
Member

Did you read the comments?

@scpike
Copy link

scpike commented Jul 25, 2014

@albus522 I'm wondering if it's possible to remove this hack entirely, now that delayed_job targets ruby 1.9.3+ (correct me if I'm wrong on that, I'm going off what's tested in travis). How familiar are you with the history of this piece of the code?

@bryckbost added this code back in 2012 (14cfa87). All the tests are still passing if I just remove what was done in 2012: https://github.com/scpike/delayed_job/compare/collectiveidea:master...remove-psych-extensions?expand=1

@albus522
Copy link
Member

I like deleting code.

@scpike
Copy link

scpike commented Jul 25, 2014

@albus522 I appreciate your commitment to code quality :). I'm going to create a PR with the removal, and would appreciate suggestions on any additional tests you'd like to see added before you're comfortable merging it.

@jarmo
Copy link
Author

jarmo commented Jul 25, 2014

+1 for deletion now that Ruby 1.8 and 1.9.2 are totally out of date.

@albus522
Copy link
Member

albus522 commented Aug 7, 2014

#679 should take care of this

@albus522 albus522 closed this Aug 7, 2014
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.

DelayedJob breaks BigDecimal deserialization from YAML
7 participants