Cleanup, Tests | MultipartIdentifier #3891
Conversation
These were always passed with the same, constant, value.
This tested a case which would never occur.
Also remove now-unnecessary test case.
Also remove a now-unnecessary test case.
Remove unnecessary IsWhitespace method. Replace always-true runtime checks against constant values with debug assertions.
This tested the exception thrown when limit is zero. This is always either 3 or 4.
…ional inclusion of MultipartIdentifier from FunctionalTests
paulmedynski
added
the
Area\Tests
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
paulmedynski
left a comment
paulmedynski
left a comment
There was a problem hiding this comment.
Another good move of some true unit tests, and valuable cleanup along the way. A few suggestions to improve the cleanup even more.
| /// <param name="ThrowOnEmptyMultipartName">If <c>true</c>, throw <see cref="ADP.InvalidMultipartName"/> if the name is whitespace.</param> | ||
| /// <param name="limit">Number of names to parse out. Defaults to four (to allow for a name formatted as [server].[database].[schema].[object].)</param> | ||
| /// <returns>An array of <paramref name="limit"/> strings containing the various parts in the identifier.</returns> | ||
| internal static string[] ParseMultipartIdentifier(string name, string property, bool ThrowOnEmptyMultipartName, int limit = MaxParts) |
There was a problem hiding this comment.
Since you're here, can you fix the capitalization of ThrowOnEmptyMultipartName to throwOnEmptyMultipartName ?
| throw ADP.InvalidMultipartNameIncorrectUsageOfQuotes(property, name); | ||
| } | ||
| Debug.Assert(limit >= 0 && limit <= MaxParts); | ||
| Debug.Assert(!ContainsChar(IdentifierStartCharacters, IdentifierSeparator)); |
There was a problem hiding this comment.
Should these move to a unit test or static constructor? There's no need to check them on every call to ParseMultipartIdentifier.
I think I would prefer a unit test, but keeping them in this file also makes sense from a sanity-check perspective.
| /// </summary> | ||
| /// <param name="name">String to parse.</param> | ||
| /// <param name="property">Name of the property containing the multipart identifier.</param> | ||
| /// <param name="ThrowOnEmptyMultipartName">If <c>true</c>, throw <see cref="ADP.InvalidMultipartName"/> if the name is whitespace.</param> |
There was a problem hiding this comment.
Hmm... this file seems to use the terms 'multipart identifier' and 'name' interchangeably. Since you're cleaning it up, would it be better to shorter 'multipart identifier' to 'identifier'?
I was confused reading this param documentation and thought 'name' refered to one of the individual parts.
|
|
||
| using System.Diagnostics; | ||
| using System.Text; | ||
|
|
There was a problem hiding this comment.
Can we #nullable enable this file?
| /// Core function for parsing the multipart identifier string. | ||
| /// </summary> | ||
| /// <param name="name">String to parse.</param> | ||
| /// <param name="property">Name of the property containing the multipart identifier.</param> |
There was a problem hiding this comment.
I think the property is only used when throwing exceptions. Could we update the docs here to indicate that it is only for diagnostic purposes, and explain a bit better what kind of property we're referring to here? Maybe 'diagnosticContext' would be less ambiguous?
|
|
||
| [Fact] | ||
| public void SingleQuotedRemoveQuote() => RunParse("[foo]", new[] { "[foo]" }, false); | ||
| public void SingleQuotedKeepQuote() => RunParse("[foo]", ["foo"]); |
There was a problem hiding this comment.
I think the old tests were named backwards. This test shows that quotes are removed.
2 participants
Description
This work performs a small cleanup of the MultipartIdentifier type, and moves its test into the UnitTests project. That simplifies some of the tech debt identified in #3890, since we can use InternalsVisibleTo to access the type directly rather than manually include the source file.
This can be reviewed commit-by-commit - I don't expect any behavioural changes. There's existing test coverage, and this passes, but everything can be verified statically. These changes essentially remove parameters which are always constant, but which were passed as a result of the MultipartIdentifier type being originally used for providers other than SqlClient.
Issues
None, but dovetails with #3890 - @benrr101 for awareness.
Testing
Existing automatic tests cover this.