Favor SqlIdentifier.getReference() instead of getReference(IdentifierProcessing)
#1458
Conversation
The method also fixed the bug of this method not honoring the IdentifierProcessing parameter, so the functionality is now identical to the toSql(IdentifierProcessing) method. Closes #1110
…es of getReference(identifierProcessing) as it did not do identifier processing, we call the getReference() method which does not do any.
kurtn718
changed the title
spring-projects-issues
added
the
status: waiting-for-triage
| } | ||
|
|
||
| @Override | ||
| @Deprecated(since="3.0.5", forRemoval = false) |
There was a problem hiding this comment.
We should deprecate that method with the next feature release, not with a bugfix. That would be since="3.1"
There was a problem hiding this comment.
And it should be forRemoval = true since we want to drop it eventually. This applies to all deprecations.
There was a problem hiding this comment.
Will change. Wasn't sure which version we wanted - and we had just done 3.0.4
| } | ||
|
|
||
| @Override | ||
| @Deprecated(since="3.0.5", forRemoval = false) |
| * Return the reference name after applying {@link IdentifierProcessing} rules. The reference name is used for | ||
| * programmatic access to the object identified by this {@link SqlIdentifier}. | ||
| * | ||
| * @deprecated Use the toSql(IdentifierProcessing processing) method instead. |
There was a problem hiding this comment.
mind the formatting, @deprecated comes after @return, we should also include that the deprecation will be since 3.1 in place (@deprecated since 3.1, use the #getReference() method instead.)
There was a problem hiding this comment.
Note that the replacement is getReference() not toSql(IdentifierProcessing)
mp911de
added
type: enhancement
mp911de
changed the title
SqlIdentifier.getReference() instead of getReference(IdentifierProcessing)
schauder
left a comment
schauder
left a comment
There was a problem hiding this comment.
I added some more comments and also chatted with Mark, so we all pull in the same direction.
Please add your changes addressing the comments as an additional commit and ping me once your done.
| * @return | ||
| * @see IdentifierProcessing#NONE | ||
| */ | ||
| default String getReference() { |
There was a problem hiding this comment.
Please give this a proper JavaDoc comment.
After talking to Mark I now again understand what the getReference vs toSql is about.
toSql is to be used when rendering an identifier in SQL, as in select id from someTable
getReference is used when accessing a column in a ResultSet here we must not apply any quoting.
This should be reflected in the JavaDoc comments of the two methods.
| * Return the reference name after applying {@link IdentifierProcessing} rules. The reference name is used for | ||
| * programmatic access to the object identified by this {@link SqlIdentifier}. | ||
| * | ||
| * @deprecated Use the toSql(IdentifierProcessing processing) method instead. |
There was a problem hiding this comment.
Note that the replacement is getReference() not toSql(IdentifierProcessing)
|
This is polished and merged. I know I wrote in some textbox, that we shouldn't change the behaviour of Anyway, I undid that change and also added you in `@author tags. |
Change to deprecate redundant getReference(IdentifierProcessing) method.
I split out the commits to make this change easier to review.
The first commit after the branch preparation outlines the changes to actual implementation. Second commit is Unit Tests supporting change, and third commit is change/eliminate all usages of the deprecated API.
I fixed the implementation of the deprecated method to actually use the IdentifierProcessing parameter. If we want to leave the implementation unchanged, please let me know.
Will squash commits after making any other needed changes.