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

Change Offer setInterval method parameter type from String to Interval.Period type #50

Closed
wants to merge 1 commit into from

Conversation

basoko
Copy link
Contributor

@basoko basoko commented Jul 28, 2014

There is an error getting an offer's interval property because the interval is not copied using the RestfulUtils.refreshInstance method. The error is due to the inconsistency between the types of interval's getter and setter methods.

@nikoloff
Copy link
Contributor

Hi @basoko,
thx for helping us to build a better software.
Is it going to work if we have 2 setters (one with String and one with Interval)?
I think leaving the String setter will simplify the usage of the wrapper.

@basoko
Copy link
Contributor Author

basoko commented Jul 29, 2014

Hi @nikoloff,

This was what I tried at first, but I got an error because there was two method with the same name and one parameter.
I decided to change the parameter type because the getter method returns that type and in other model's setters you use the specific type instead of a String, like for example in the Subscription model with interval property of type .

I think that another solution, if you don't want to change the setter parameter type, would be to implement a converter from Apache Commons Beanutils, but I think that this is better.

@stoilkov
Copy link
Contributor

stoilkov commented Oct 2, 2014

thanks @basoko this was merged manually with #60

@stoilkov stoilkov closed this Oct 2, 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.

3 participants