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

Support multi-member annotations. #89

Merged
merged 1 commit into from
Dec 2, 2014

Conversation

JakeWharton
Copy link
Member

Fixes #76.

@@ -57,6 +57,16 @@ public Appendable write(Appendable appendable, Context context) throws IOExcepti
appendable.append(onlyEntry.getKey()).append(" = ");
}
onlyEntry.getValue().write(appendable, context);
} else {
boolean first = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

As a simpler alternative:

String sep = "";
for (Entry<String, Writable> entry : memberMap.entrySet()) {
  appendable.append(sep).append(entry.getKey()).append(" = ");
  entry.getValue().write(appendable, context);
  sep = ", ";
}

import java.util.Set;

/** A context which always returns fully-qualified type names. */
final class EmptyContext implements Writable.Context {
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's an equivalent such context in Writeables; maybe there can be reuse here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It works for this case, but I don't think it will work for a larger-scope testing strategy where we want to verify the actual output due to its createSubcontext behavior (throwing exception).

Copy link
Collaborator

Choose a reason for hiding this comment

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

What I meant is EmptyContext (with its createSubcontext() could be moved out of src/test and used in Writeables.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see. Yeah you're right. For now I'll switch. When we start testing more we'll know what to change.

swankjesse added a commit that referenced this pull request Dec 2, 2014
@swankjesse swankjesse merged commit c4bc7c3 into javawriter_3 Dec 2, 2014
@JakeWharton JakeWharton deleted the jw/multimember-annotation branch December 3, 2014 22:21
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.

4 participants