Fix New-ModuleManifest handling of uri's#3631
Conversation
dantraMSFT
left a comment
dantraMSFT
left a comment
There was a problem hiding this comment.
Wrap the return statement in brackets
There was a problem hiding this comment.
The method comments should be updated to reflect string or URI.
| BuildPrivateDataInModuleManifest(result, streamWriter); | ||
|
|
||
| BuildModuleManifest(result, "HelpInfoURI", Modules.HelpInfoURI, !string.IsNullOrEmpty(_helpInfoUri), () => QuoteName(_helpInfoUri), streamWriter); | ||
| BuildModuleManifest(result, "HelpInfoURI", Modules.HelpInfoURI, !string.IsNullOrEmpty(_helpInfoUri), () => QuoteName((null != _helpInfoUri) ? new Uri(_helpInfoUri) : null), streamWriter); |
There was a problem hiding this comment.
This statement is getting rather convoluted. Suggest breaking out the QuoteName logic for readability.
There was a problem hiding this comment.
Since QuoteName() takes a null but Uri constructor doesn't accept null, I think we'd end up with a bunch of code that makes it harder to read in this particular case.
| Dbg.Assert(!String.IsNullOrWhiteSpace(parameterName), "parameterName should not be null or whitespace"); | ||
|
|
||
| if (uri != null && !Uri.IsWellFormedUriString(uri.ToString(), UriKind.Absolute)) | ||
| if (uri != null && !Uri.IsWellFormedUriString(uri.AbsoluteUri, UriKind.Absolute)) |
There was a problem hiding this comment.
You're using inconsistent comparison to null ordering. (i.e., value != null versus null != value). Suggest picking one.
There was a problem hiding this comment.
I prefer null on the left, but most of the code in this file has it on the right, I'll change my usage to make it consistent
| { | ||
| if (name == null) | ||
| return "''"; | ||
| else if (name is Uri) |
There was a problem hiding this comment.
Avoid casting multiple times. consider assigning name as Uri and testing for null.
There was a problem hiding this comment.
In this case, name may or may not be a Uri. I'm special casing when it is a Uri, I want to use the AbsoluteUri member value.
There was a problem hiding this comment.
The method comments should be updated to reflect string or URI. Better would be to have two explicit overloads and remove the 'object' overload since I don't think anything other than uri and string are expected.
There was a problem hiding this comment.
I looked through the calls to QuoteName() and I believe you are right that it's only a string or Uri. I'll separate it.
There was a problem hiding this comment.
Actually Version is another type that needs to be supported
SteveL-MSFT
force-pushed
the
new-modulemanifest-uri
branch
3 times, most recently
from
3c75047 to
5adb431
Compare
…by using ToString() which just outputs the original string. If the string was a uri with spaces, ToString() doesn't return the escaped version. The AbsoluteUri property should be used instead which returns an escaped absolute uri (if valid). Also renamed TestModuleManfest.ps1 to TestModuleManifest.Tests.ps1 so that it gets picked up correctly as Pester test. Since HelpInfoUri is just a string, ensure it is a valid absolute uri and escaped correctly whereas before it was just an opaque string that wasn't validated.
SteveL-MSFT
force-pushed
the
new-modulemanifest-uri
branch
from
5adb431 to
5017f79
Compare
5 participants
New-ModuleManifest was incorrectly checking if a Uri was well formed by using ToString() which just outputs the original string. If the string was a uri with spaces, ToString() doesn't return the escaped version. The AbsoluteUri property should be used instead which returns an escaped absolute uri (if valid).
Also renamed TestModuleManfest.ps1 to TestModuleManifest.Tests.ps1 so that it gets picked up correctly as Pester test.
Since HelpInfoUri is just a string, ensure it is a valid absolute uri and escaped correctly whereas before it was just an opaque string that wasn't validated.
Addresses #3336