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

Map More than one name token to presenter #346

Merged
merged 3 commits into from Mar 7, 2014

Conversation

igieon
Copy link
Contributor

@igieon igieon commented Nov 20, 2013

Change annotation NameToken value to array of name tokens. Change
PlaceImpl. For tab content proxy just use first nametoken.

Second implementation.

@christiangoudreau
Copy link
Member

@PhilBeaudoin

@@ -134,7 +134,7 @@ private void findNameToken(JClassType proxyInterface)
+ NameToken.class.getSimpleName() + ".", null);
throw new UnableToCompleteException();
}
nameToken = nameTokenAnnotation.value();
nameTokens = nameTokenAnnotation.value();
Copy link
Member

Choose a reason for hiding this comment

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

Rename method to findNameTokens. I would also add something to make sure there is at least one NameToken ie:

if (nameTokens.size() == 0) {
    logger.log(TreeLogger.ERROR,
            "The proxy for '" + presenterInspector.getPresenterClassName() + "' is annotated with '@"
                    + NameToken.class.getSimpleName() + "', but has no name token specified.", null);
    throw new UnableToCompleteException();
}

@Chris-V
Copy link
Member

Chris-V commented Nov 22, 2013

We must make sure there is at least one name token, otherwise this looks good to me. Good job :)

@igieon
Copy link
Contributor Author

igieon commented Nov 26, 2013

Coudl I close pull request #343 other implementation ?

@christiangoudreau
Copy link
Member

Yes, close the other one. Before going any further with this one, I want @PhilBeaudoin to review at least.

@branflake2267
Copy link
Contributor

Another thought to this addition, could be useful for those with multiple languages, or support multiple languages in the with the token. Do you think that would be useful?

@igieon
Copy link
Contributor Author

igieon commented Dec 18, 2013

Any news about include this patch ? Need probide other changes

@christiangoudreau
Copy link
Member

@PhilBeaudoin needs to review this. He's currently in a deadline rush with Chrome.

You can provide other PR on top of this one if you have done a new branch on top of this one by comparing the new branch against this branch if you want.

Change annotation NameToken value to array of name tokens. Change
PlaceImpl. For tab content proxy just use first nametoken.
Add dummy javadoc (fix checkstyle)
@Chris-V
Copy link
Member

Chris-V commented Mar 6, 2014

Thanks for maintaining this PR and sorry for the very long wait. Please remove all the @author tags introduced in c531116 and I will proceed to the merge. The author is stored in the commit information.

@christiangoudreau
Copy link
Member

Chris, we need those changes, do it once you're done with your other tasks

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.

None yet

4 participants