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

Allow collection or array object on item attribute of foreach element #1075

Conversation

kazuki43zoo
Copy link
Member

Fixes gh-1074

Please review this.

@@ -154,7 +154,7 @@ public void appendSql(String sql) {
GenericTokenParser parser = new GenericTokenParser("#{", "}", new TokenHandler() {
@Override
public String handleToken(String content) {
String newContent = content.replaceFirst("^\\s*" + item + "(?![^.,:\\s])", itemizeItem(item, index));
String newContent = content.replaceFirst("^\\s*" + item + "(?![^.\\[,:\\s])", itemizeItem(item, index));
if (itemIndex != null && newContent.equals(content)) {
newContent = content.replaceFirst("^\\s*" + itemIndex + "(?![^.,:\\s])", itemizeItem(itemIndex, index));
Copy link
Member

Choose a reason for hiding this comment

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

Is there any particular reason why the same change is not applied to this line?

Copy link
Member Author

@kazuki43zoo kazuki43zoo Aug 15, 2017

Choose a reason for hiding this comment

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

@harawata Thanks for your review!
The itemIndex is variable name for representing a integer value. In other words, there is no case that it have a array/collection object. Therefore I think it not need applying same change.

WDYT ?

Copy link
Member

@harawata harawata Aug 15, 2017

Choose a reason for hiding this comment

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

When a map is passed as collection, index represents the key of each entry.
So, technically, it can be a list or array, I think.
(I haven't had time to verify it)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh ... I missed it.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah. Although it's a neat feature, not many users need it, I guess :)

@kazuki43zoo
Copy link
Member Author

Hi @harawata , I withdraw this PR because i found more considerations are need.

@harawata
Copy link
Member

OK. Thank you for your work!

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.

2 participants