Issue #4 - Clarify manual submission doc in Usage.md #5
Conversation
|
Hi @lisaong, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
TTYL, MSBOT; |
|
Thanks for doing this, Lisa! Overall, this looks good. The main issue with this is that One other request, while you're in that section, is to fix the code in the first bulletpoint. Not sure why I had |
|
Done and done. I tested the export of DeepCopy-Object by comparing the behavior before and after the export. |
|
Great, thanks for the update. Since this is a documentation update for clarity, I'm going to be a bit more picky here....I'm noticing one other thing in the documentation now too, and it's not your fault, but hopefully you can fix it. The example code in the bullet points is not getting formatted as a code snippet (not in your new bullet points, nor in the other existing bullet points within that section). I think yours need another 3 spaces indentation before they get recognized as code snippets. In other markdown editors, I'm seeing the existing bullet point code snippets show up right, but not when displayed in GitHub for some reason. Thoughts? Also, as I'm not sure if I can modify your pull request when I accept it, can you go ahead and update the version number in Thanks once more. |
| @@ -253,6 +253,7 @@ function DeepCopy-Object | |||
| #> | |||
| { | |||
| [CmdletBinding()] | |||
| [Alias('DeepCopy-Object')] | |||
| [Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSUseApprovedVerbs", "", Justification="Intentional. This isn't exported, and needed to be explicit relative to Copy-Object.")] | |||
There was a problem hiding this comment.
Now that you're using an approved verb by default, we no longer need to suppress this code analysis message since it won't show up. Please remove this, and then run static analysis again and make sure it's still clean.
Removed quotation marks from examples for consistency More fun with quotes Addressed feedback from @HowardWolosky-MSFT: - exported DeepCopy-Object (as an alias) - Fixed 1 unrelated typo Addressed feedback from @HowardWolosky-MSFT: - removed code analysis suppression that's made redudant, now that DeepCopy-Object is aliased to an approved verb - fixed indentation of bulletted code snippets version bump to 1.0.1 Update USAGE.md Trying out indentation changes again for bulleted code snippets, using Github's editor
|
Updated and re-squashed commits. There were a bunch of existing warnings from Invoke-ScriptAnalyzer that were unrelated to my changes, I diff'ed of the output of the original pre-renamed version without the suppression and verified that that specific warning around CopyObject went away. |
|
This final update looks great. I'll approve it and pull it in. I'm interested in hearing more about the static analysis warnings that you were seeing. It was clean before I submitted v1.0.0 to GitHub, and I just ran it again on what's in master and it's still clean. I also ran it against your branch and it was clean. It's possible that you had other modules loaded at the time that you ran the static analysis on StoreBroker which resulted in you getting some false positives. |
#4