8000 #1203: Modified AbstractPrefField interface to include generic get and put methods. by chvndb · Pull Request #1272 · androidannotations/androidannotations · GitHub
[go: up one dir, main page]
Skip to content
This repository was archived by the owner on Feb 26, 2023. It is now read-only.

#1203: Modified AbstractPrefField interface to include generic get and put methods. #1272

Merged
WonderCsabo merged 1 commit into from
Jan 9, 2015
Merged

#1203: Modified AbstractPrefField interface to include generic get and put methods. #1272

WonderCsabo merged 1 commit into from
Jan 9, 2015

Conversation

@chvndb
Copy link
Contributor
@chvndb chvndb commented Dec 10, 2014

Relates to #1203

@WonderCsabo
Copy link
Member
WonderCsabo commented Dec 10, 2014

Why did you close the previous PR? You could overwrite the branch with git push -f.

@dodgex
Copy link
Member
dodgex commented Dec 10, 2014

i think because he had a wrong branch name there. the other was 203-Generic_AbstractPrefField instead of 1203. or does git/github support branch renaming for a PR?

@dodgex
Copy link
Member
dodgex commented Dec 10, 2014

@chvndb you should add @Override annotations.

@chvndb
Copy link
Contributor Author
chvndb commented Dec 10, 2014

Indeed, the branch name was incorrect. I added @Override annotations.

@WonderCsabo
Copy link
Member
WonderCsabo commented Dec 10, 2014

OK, branch renaming is not supported I think. I'll review this later.

@WonderCsabo
Copy link
Member
WonderCsabo commented Dec 10, 2014

What happens if one passes null? I think that will yield NPE while unboxing.

@chvndb
Copy link
Contributor Author
chvndb commented Dec 10, 2014

Ah indeed, we had this discussion before and I forgot about it. We agreed on setting the preference to its default value. I will add this.

@chvndb
Copy link
Contributor Author
chvndb commented Dec 11, 2014

something like this?

@WonderCsabo
Copy link
Member
WonderCsabo commented Dec 11, 2014

I do not think so. I think we agreed on to add zero if one passes null etc.

@chvndb
Copy link
Contributor Author
chvndb commented Dec 11, 2014

re-reading the original issue, it is ambiguous what the action should be when passing null. What do you actually mean by adding zero?

@WonderCsabo
Copy link
Member
WonderCsabo commented Dec 11, 2014 
if (value == null) {
  putLong(0);
}

etc

@chvndb
Copy link
Contributor Author
chvndb commented Dec 11, 2014

I see, so you would set the preference to what you consider to be the default value for the given type. I think it is more logical to set it to the default value the developer has defined, as he has thought about what the default value should be in the context of his application.

@WonderCsabo
Copy link
Member
WonderCsabo commented Dec 11, 2014

Maybe you are right, but i am not really sure what would be better. @yDelouis WDYT?

@chvndb
Copy link
Contributor Author
chvndb commented Dec 11, 2014

E.g. storing a json object in a string preference and the default value is {"name":"chvndb"}. Using put(null) in the first case would set it to the empty string. However, this will most likely break the application while resetting it to the original will not.

@yDelouis
Copy link
Contributor
yDelouis commented Dec 12, 2014

I think chvndb is right, we should reset the value to the default one.
Setting a value to null in a preference is restoring the default value for it. So, 8000 in our case, we should do the same.

@WonderCsabo
Copy link
Member
WonderCsabo commented Dec 15, 2014

@chvndb please modify the null behavior as @yDelouis suggested. Also the reset() method is not needed in that case, i think. Also please squash your commits into one.

@chvndb
Copy link
Contributor Author
chvndb commented Dec 16, 2014

Added the null behaviour to reset the preference to the user defined default.

Added the reset method as it is a cleaner API. It allows a user to explicitly reset a given preference without using the side effect of putting a null value. I can still remove it if you want.

(p.s. squashed the commits)

@WonderCsabo
Copy link
Member
WonderCsabo commented Dec 16, 2014

I think the reset() method is redundant. We already have the remove() method, and after that get() returns the default value.

@WonderCsabo
Copy link
Member
WonderCsabo commented Dec 21, 2014

Thanks! Can you reword the commit as to follow the standard git format (as laid out in the contribution guide)?

@chvndb
Copy link
Contributor Author
chvndb commented Dec 22, 2014

Like this?

@WonderCsabo
Copy link
Member
WonderCsabo commented Dec 22, 2014

Yeah, thanks.

WonderCsabo
WonderCsabo reviewed Dec 22, 2014
View reviewed changes
...ations-api/src/main/java/org/androidannotations/api/sharedpreferences/AbstractPrefField.java Outdated
Copy link
Member
@WonderCsabo WonderCsabo Dec 22, 2014

Choose a reason for hiding this c 8000 omment

The reason will be displayed to describe this comment to others. Learn more.

final?

@WonderCsabo
Copy link
Member
WonderCsabo commented Jan 8, 2015

@chvndb can you you please react to the review comments?

@chvndb
Add generic AbstractPrefField
58fbbf8 
Write null to preference resets the preference to the user defined default value.
@chvndb
Copy link
Contributor Author
chvndb commented Jan 8, 2015

Sorry, with the new year I guess I missed your comments. Updated.

On 08 Jan 2015, at 13:26, Csaba Kozák notifications@github.com wrote:

@chvndb https://github.com/chvndb can you you please react to the review comments?

—
Reply to this email directly or view it on GitHub #1272 (comment).

WonderCsabo added a commit that referenced this pull request Jan 9, 2015
@WonderCsabo
Merge pull request #1272 from chvndb/1203-Generic_AbstractPrefField 67E6
eabff92 
#1203: Modified AbstractPrefField interface to include generic get and put methods.
@WonderCsabo WonderCsabo merged commit eabff92 into androidannotations:develop Jan 9, 2015
@WonderCsabo
Copy link
Member
WonderCsabo commented Jan 9, 2015

Thanks!

@WonderCsabo WonderCsabo added this to the 3.3 milestone Apr 30, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

3.3

Development

Successfully merging this pull request may close these issues.

4 participants

@chvndb @WonderCsabo @dodgex @yDelouis
0