Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings
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 into
androidannotations:developandroidannotations/androidannotations:developfrom
ironjan:1877-add-backticks-to-sharedpreferences-javadocironjan/androidannotations:1877-add-backticks-to-sharedpreferences-javadocCopy head branch name to clipboard
Oct 29, 2016
Merged

Clarified javadocs in @SharedPrefs with backticks#1878
dodgex merged 1 commit into
androidannotations:developandroidannotations/androidannotations:developfrom
ironjan:1877-add-backticks-to-sharedpreferences-javadocironjan/androidannotations:1877-add-backticks-to-sharedpreferences-javadocCopy head branch name to clipboard

Conversation

@ironjan

@ironjan ironjan commented Oct 16, 2016 •
edited
Loading

Copy link
Copy Markdown 
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

dodgex commented Oct 17, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

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

dodgex commented Oct 17, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

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

@WonderCsabo

WonderCsabo commented Oct 17, 2016

Copy link
Copy Markdown
Member

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

@ironjan

ironjan commented Oct 17, 2016

Copy link
Copy Markdown
Author

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

@ironjan

ironjan commented Oct 24, 2016

Copy link
Copy Markdown
Author

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

dodgex commented Oct 25, 2016

Copy link
Copy Markdown
Member

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

@dodgex

dodgex commented Oct 26, 2016

Copy link
Copy Markdown
Member

@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

ironjan commented Oct 26, 2016 •
edited
Loading

Copy link
Copy Markdown
Author

Copyright notice added and branch rebased.

WonderCsabo
WonderCsabo suggested changes Oct 26, 2016
View reviewed changes

@WonderCsabo WonderCsabo left a comment

Copy link
Copy Markdown
Member

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.

Comment thread ...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);

@WonderCsabo WonderCsabo Oct 26, 2016

Copy link
Copy Markdown
Member

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.

@dodgex dodgex Oct 27, 2016

Copy link
Copy Markdown
Member

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

@WonderCsabo WonderCsabo Oct 27, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Of course!

Comment thread ...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");

@WonderCsabo WonderCsabo Oct 26, 2016

Copy link
Copy Markdown
Member

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()...

Comment thread ...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

@WonderCsabo WonderCsabo Oct 26, 2016

Copy link
Copy Markdown
Member

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

ironjan commented Oct 28, 2016

Copy link
Copy Markdown
Author

@WonderCsabo I applied the suggestions and squashed everything together.

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

dodgex commented Oct 29, 2016

Copy link
Copy Markdown
Member

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
Morty Proxy This is a proxified and sanitized view of the page, visit original site.