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

fix the description of set tag in dynamic-sql.xml doc #2347

Closed
wants to merge 5 commits into from
Closed

fix the description of set tag in dynamic-sql.xml doc #2347

wants to merge 5 commits into from

Conversation

xtyuns
Copy link
Contributor

@xtyuns xtyuns commented Sep 26, 2021

in the fact, <set> is equal to <trim prefix="set" prefixOverrides="," suffixOverrides=",">

ref: 95166a6#diff-df20f6a8f86025feb42cead35070d7689a4f04afc051008c93a2f38250bf53afR31

@xtyuns xtyuns marked this pull request as ready for review September 26, 2021 06:19
@harawata
Copy link
Member

Thank you for the PR, @xtyuns !

Usually, you only need prefixOverrides or suffixOverrides, not both.
And, in this particular example, suffixOverrides is sufficient.

@harawata harawata closed this Mar 21, 2022
@xtyuns
Copy link
Contributor Author

xtyuns commented Mar 22, 2022

Hi @harawata, I understand what you mean, but what I pointed out is about the replacement of set tags and trim tags.
Please see the source of set tag:

public class SetSqlNode extends TrimSqlNode {
private static final List<String> COMMA = Collections.singletonList(",");
public SetSqlNode(Configuration configuration,SqlNode contents) {
super(configuration, contents, "SET", COMMA, null, COMMA);
}
}

Because I encountered some problems in the process of using MyBatis, so I like to point out the errors in the documentation.

@harawata
Copy link
Member

harawata commented Mar 22, 2022

Hi @xtyuns ,

I see.
The behavior of <set /> tag was changed via #21 , actually.
So, the sentence might be a little bit confusing.

I am OK with rephrasing the sentence. For example:

Alternatively, you can achieve the same effect by using trim element.

I just don't think specifying both prefixOverrides and suffixOverrides is a good example.

I have reopened the PR, so you can add or force-push new commits to the same branch (patch-1).
If you prefer, you can close this PR and open a new PR.

Thank you!

@harawata harawata reopened this Mar 22, 2022
@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 87.283% when pulling 0d27a42 on xtyuns:patch-1 into ea4e829 on mybatis:master.

@xtyuns
Copy link
Contributor Author

xtyuns commented Mar 22, 2022

Ok, I open a new PR, so I closed this one.

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

3 participants