Obsolete unused "xml" type methods #1771
Conversation
fredericDelaporte
force-pushed
the
NullableToStringFromString
branch
3 times, most recently
from
fd73f5f to
22a6012
Compare
| @@ -28,6 +28,16 @@ public partial interface IVersionType : IType | |||
| /// </summary> | |||
| IComparer Comparator { get; } | |||
|
|
|||
| // 6.0 TODO: rename "xml" parameter as "value": it is not a xml string. The fact it generally comes from a xml | |||
| // attribute value is irrelevant to the method behavior. | |||
| /// <summary> | |||
There was a problem hiding this comment.
The value is de-serialized by .Net, it is not a xml encoded value.
| /// <returns>The value.</returns> | ||
| /// <remarks>Notably meant for parsing <c>unsave-value</c> mapping attribute value. Contrary to what could | ||
| /// be expected due to its current name, <paramref name="xml"/> must be a plain string, not a xml encoded | ||
| /// string.</remarks> | ||
| object FromStringValue(string xml); |
There was a problem hiding this comment.
This method uses the same name than NullableType.FromStringValue, which has no more usages and is obsoleted. This causes IVersionType types to override an obsoleted method with a non-obsoleted one.
It has the same purpose than IIdentifierType.StringToObject indeed. It would have been better to have the same name.
| @@ -5,17 +5,21 @@ namespace NHibernate.Type | |||
| /// </summary> | |||
| public interface IIdentifierType : IType | |||
| { | |||
| // 6.0 TODO: rename "xml" parameter as "value": it is not a xml string. The fact it generally comes from a xml | |||
| // attribute value is irrelevant to the method behavior. | |||
There was a problem hiding this comment.
The value is de-serialized by .Net, it is not a xml encoded value.
| /// <para> | ||
| /// It has been "sealed" because the Types inheriting from <see cref="NullableType"/> | ||
| /// do not need and should not override this method. All of their implementation | ||
| /// should be in <see cref="ToString(object)"/>. |
There was a problem hiding this comment.
So a thing meant to log plain text is implemented by calling a thing documented as meant to generate xml strings...
| [Obsolete("This method was not used for parsing xml strings, but instead used for parsing already de-encoded " + | ||
| "strings originating from xml (so indeed, plain text strings). It will be renamed \"FromString\" in a " + | ||
| "future version. Implement a \"object FromString(string value)\" without any xml de-coding, called by your" + | ||
| "\"object FromXMLString(string xml)\", in order to get ready for the rename.")] | ||
| object FromXMLString(string xml); |
There was a problem hiding this comment.
The supplied xml string is never xml encoded, contrary to what we could expect with such naming and documentation. So I have considered a name change is required there. I do not see a better pattern for renaming an interface method than the one showcased here.
| // Since 5.2 | ||
| [Obsolete("This method was not used in a xml context, but instead just used for logs. It will be renamed " + | ||
| "\"ToString\" in a future version. Implement a \"string ToString(object value)\", called by your " + | ||
| "\"string ToXMLString(object value)\", in order to get ready for the rename.")] | ||
| string ToXMLString(object value); |
There was a problem hiding this comment.
This one is less prone to incite users to code bugs: since it is used only for logs: it will not be a serious issue if the user xml-encodes the value. But for consistency, renaming too.
fredericDelaporte
force-pushed
the
NullableToStringFromString
branch
from
22a6012 to
9ff2f41
Compare
|
Rebased for solving conflicts with #1765. |
Resolving conflicts is not enough...
This is a follow up to #579 which has removed most of the xml entity mode partial implementation. NHibernate types were still having methods documented as meant to parse from xml or convert to xml values. Theses methods:
This PR obsolete unused cases. The remaining used cases are of two kinds: implementation of
IVersionType, or usage by "opportunity". The former cases are not obsoleted. The later cases are obsoleted.(This subject was triggered by the discussion on #1765.)