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

Update the expression of set tags in Dynamic SQL document. #2484

Merged
merged 1 commit into from
Mar 25, 2022
Merged

Update the expression of set tags in Dynamic SQL document. #2484

merged 1 commit into from
Mar 25, 2022

Conversation

xtyuns
Copy link
Contributor

@xtyuns xtyuns commented Mar 22, 2022

The behavior of set tags was changed via #21

@xtyuns xtyuns changed the title Alternatively, you can achieve the same effect by using trim element. Update the expression of set tags in Dynamic SQL document. Mar 22, 2022
@harawata
Copy link
Member

Thank you for the update, @xtyuns .

I'm sorry, my comment on #2347 was confusing.
What I suggested was to change the explanation instead of the XML code. Like this:

diff --git a/src/site/xdoc/dynamic-sql.xml b/src/site/xdoc/dynamic-sql.xml
index abc81bffcb..88f89d0086 100644
--- a/src/site/xdoc/dynamic-sql.xml
+++ b/src/site/xdoc/dynamic-sql.xml
@@ -135,7 +135,7 @@ AND title like ‘someTitle’]]></source>
   where id=#{id}
 </update>]]></source>
   <p>Here, the <em>set</em> element will dynamically prepend the SET keyword, and also eliminate any extraneous commas that might trail the value assignments after the conditions are applied.</p>
-  <p>If you’re curious about what the equivalent <em>trim</em> element would look like, here it is:</p>
+  <p>Alternatively, you can achieve the same effect by using <em>trim</em> element:</p>
   <source><![CDATA[<trim prefix="SET" suffixOverrides=",">
   ...
 </trim>]]></source>

We should not add prefixOverrides here because it's unnecessary.
And the new explanation does not say that <trim prefix="SET" suffixOverrides=","> is equivalent to <set />.

If you think it's still confusing, let's just remove this additional <trim> example.
<trim /> is already explained earlier.

diff --git a/src/site/xdoc/dynamic-sql.xml b/src/site/xdoc/dynamic-sql.xml
index abc81bffcb..676dc75336 100644
--- a/src/site/xdoc/dynamic-sql.xml
+++ b/src/site/xdoc/dynamic-sql.xml
@@ -135,11 +135,6 @@ AND title like ‘someTitle’]]></source>
   where id=#{id}
 </update>]]></source>
   <p>Here, the <em>set</em> element will dynamically prepend the SET keyword, and also eliminate any extraneous commas that might trail the value assignments after the conditions are applied.</p>
-  <p>If you’re curious about what the equivalent <em>trim</em> element would look like, here it is:</p>
-  <source><![CDATA[<trim prefix="SET" suffixOverrides=",">
-  ...
-</trim>]]></source>
-  <p>Notice that in this case we’re overriding a suffix, while we’re still appending a prefix.</p>
   </subsection>
   <subsection name="foreach">
   <p>Another common necessity for dynamic SQL is the need to iterate over a collection, often to build an IN condition. For example:</p>

@xtyuns
Copy link
Contributor Author

xtyuns commented Mar 23, 2022

You are correct, adding prefixOverrides and suffixOverrides at the same time may indeed cause confusion for users. So I prefer to modify it to your first solution: modify the equivalent to achieve the same effect

@xtyuns
Copy link
Contributor Author

xtyuns commented Mar 24, 2022

Hi @harawata, If you don't mind, I force-push the latest submission. Or I can create a new RP.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 87.224% when pulling 63de279 on xtyuns:master into 9ee6941 on mybatis:master.

@harawata harawata merged commit a3576e6 into mybatis:master Mar 25, 2022
@harawata
Copy link
Member

Thank you for the update, @xtyuns !
Nice work on translations as well 👍

@kazuki43zoo kazuki43zoo added the documentation Indicates a changing on documentation(reference or javadoc) label May 15, 2022
@kazuki43zoo kazuki43zoo added this to the 3.5.10 milestone May 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Indicates a changing on documentation(reference or javadoc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants