Skip to content
This repository was archived by the owner on Feb 26, 2023. It is now read-only.

Clarified javadocs in @SharedPrefs with backticks#1878

Merged
dodgex merged 1 commit intoandroidannotations:developfrom
ironjan:1877-add-backticks-to-sharedpreferences-javadoc
Oct 29, 2016
Merged

Clarified javadocs in @SharedPrefs with backticks#1878
dodgex merged 1 commit intoandroidannotations:developfrom
ironjan:1877-add-backticks-to-sharedpreferences-javadoc

Conversation

@ironjan
Copy link

@ironjan ironjan commented Oct 16, 2016 •
edited
Loading 

Previously, "<p><b>Defaults to</b>: </p>" was generated for string fields with
an empty default value. This commit inserts back-ticks so that the default
value in the javadoc is easier to be identified:

 * "<p><b>Defaults to</b>: ""</p>" for empty default strings
 * "<p><b>Defaults to</b>: "value"</p>" for @DefaultString("value")

I had to wrap this in code tags because the b-tags made the message unreadable

@ironjan ironjan mentioned this pull request Oct 16, 2016
Closed
@dodgex
Copy link
Member

dodgex commented Oct 17, 2016 •
edited
Loading

Hello @ironjan,

thank you for this PR.

I have some points:

  • could you please add Copyright (C) 2016 the AndroidAnnotations project to the copyright header (as shown here) to all your modified files?
  • could you please check if this is the same for JavaDoc generation of @Extra and @FragmentArg? (added in this PR)

thanks

@dodgex
Copy link
Member

dodgex commented Oct 17, 2016 •
edited
Loading

okay. ignore the 2nd point. I just realized that we are not adding a value in those javadoc comments.

@WonderCsabo
Copy link
Member

WonderCsabo commented Oct 17, 2016

Thanks for the PR! IMHO, it would be much cleaner, if you would add quotes, and only around string default values.

@ironjan
Copy link
Author

ironjan commented Oct 17, 2016

Thanks for the feedback. I'll add the copyright and adapt this to add quotes only around string default values.

@ironjan
Copy link
Author

ironjan commented Oct 24, 2016

I'm not sure if I chose the correct way to check for StringFields. If the changes look good, I'll rebase this PR.

@dodgex
Copy link
Member

dodgex commented Oct 25, 2016

@ironjan could you please add the update copyright in SharedPrefWithJavaDoc.java`?

@dodgex
Copy link
Member

dodgex commented Oct 26, 2016

@ironjan also it would be good to rebase your branch with the latest changes from our develop as it has some changes to the pom.xml that allow the build pass on travis due to the license header changes.

@ironjan ironjan force-pushed the 1877-add-backticks-to-sharedpreferences-javadoc branch from 39fa28c to 134324c Compare October 26, 2016 21:06
@ironjan
Copy link
Author

ironjan commented Oct 26, 2016 •
edited
Loading

Copyright notice added and branch rebased.

WonderCsabo
WonderCsabo suggested changes Oct 26, 2016
View reviewed changes
Copy link
Member

@WonderCsabo WonderCsabo left a comment

Choose a reason for hiding this comment

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

Looks good, can you address my comments? Also please squash the commits into one. No need to add copyright headers in a separate commit.

...ns-core/androidannotations/src/main/java/org/androidannotations/holder/SharedPrefHolder.java Outdated

if (defaultValueStr != null) {
fieldMethod.javadoc().append("<p><b>Defaults to</b>: " + defaultValueStr + "</p>\n");
boolean isStringPrefField = "stringField".equals(fieldHelperMethodName);
Copy link
Member

@WonderCsabo WonderCsabo Oct 26, 2016

Choose a reason for hiding this comment

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

Maybe a little bit nicer check would be prefFieldHelperClass == StringSetPrefField.class. So we at least do not rely on magic string constants.

Copy link
Member

@dodgex dodgex Oct 27, 2016

Choose a reason for hiding this comment

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

Shouldnt it be StringPrefField.class instead of StringSetPrefField.class

Copy link
Member

@WonderCsabo WonderCsabo Oct 27, 2016

Choose a reason for hiding this comment

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

Of course!

...ns-core/androidannotations/src/main/java/org/androidannotations/holder/SharedPrefHolder.java Outdated
fieldMethod.javadoc().append("<p><b>Defaults to</b>: " + defaultValueStr + "</p>\n");
boolean isStringPrefField = "stringField".equals(fieldHelperMethodName);
if (isStringPrefField) {
fieldMethod.javadoc().append("<p><b>Defaults to</b>: \"" + defaultValueStr + "\"</p>\n");
Copy link
Member

@WonderCsabo WonderCsabo Oct 26, 2016

Choose a reason for hiding this comment

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

Can you avoid the code duplication? Extract the only part which is different, to a variable.

For example:

String defaultValueJavaDoc;

if (isStringPrefField) {
  defaultValueJavaDoc = "\"" + defaultValueStr + "\"";
} else {
  ...
}

fieldMethod.javadoc()...

...nnotations/src/main/java/org/androidannotations/internal/core/handler/SharedPrefHandler.java Outdated
@@ -1,5 +1,6 @@
/**
* Copyright (C) 2010-2016 eBusiness Information, Excilys Group
* Copyright (C) 2016 the AndroidAnnotations project
Copy link
Member

@WonderCsabo WonderCsabo Oct 26, 2016

Choose a reason for hiding this comment

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

Remove this line, as you did not edited changed file.

@ironjan
Clarified javadoc in @SharedPrefs with quotes
f1df7e5 
Previously, "<p><b>Defaults to</b>: </p>" was generated for string fields with
an empty default value. This commit inserts quotes so that the default
value in the javadoc is easier to be identified:

 * "<p><b>Defaults to</b>: \"\"</p>" for empty default strings
 * "<p><b>Defaults to</b>: \"value\"</p>" for @DefaultString("value")
@ironjan ironjan force-pushed the 1877-add-backticks-to-sharedpreferences-javadoc branch from 134324c to f1df7e5 Compare October 28, 2016 21:42
@ironjan
Copy link
Author

ironjan commented Oct 28, 2016

@WonderCsabo I applied the suggestions and squashed everything together.

@dodgex dodgex merged commit c568347 into androidannotations:develop Oct 29, 2016
@dodgex
Copy link
Member

dodgex commented Oct 29, 2016

Thank you! :)

@ironjan ironjan deleted the 1877-add-backticks-to-sharedpreferences-javadoc branch October 30, 2016 16:16
@dodgex dodgex added this to the 4.2 milestone Dec 11, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@dodgex dodgex dodgex left review comments

@WonderCsabo WonderCsabo WonderCsabo approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

4.2

Development

Successfully merging this pull request may close these issues.

3 participants

@ironjan @dodgex @WonderCsabo