-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add SnippetGenerator #7928
Add SnippetGenerator #7928
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great to see. We see outdated code in readmes all the time. Something to consider is building this into an open source project that the sdk uses - as I know lots of other folks in the world would use it. We should also consider how we do this across all SDKs.
$generatorProject = "$PSScriptRoot\SnippetGenerator\SnippetGenerator.csproj"; | ||
dotnet build $generatorProject | ||
|
||
foreach ($file in Get-ChildItem "$PSScriptRoot\..\sdk" -Filter README.md -Recurse) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we considered potentially keeping snippets in their own files, so that each can be a small and contained test. I'm not a fan of regions
, personally, and worry about polluting the sample code with them will make things less clear to the casual reader.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you want to inject the entire file with headers and everything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking more of keeping the region-based approach, but segmenting it from the samples proper. In some form that can be run and validated as part of the nightly process, but maybe a set of tests in its own folder or something similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm open to improvements but wanted to start with something simple
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that mine is the best suggestion either. One of the goals that we had discussed was helping developers understand the copy/paste worthy areas of the samples, which the regions do - if (and only if) you understand the convention.
Too bad that we can't create our own alias for region
and use something like:
var code = ...
#snippet Do the thing
// Stuff
#endsnippet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that @danieljurek is planning to start digging into samples and help define a convention so that they can essentially be standalone examples and can be ran during our smoke testing. Personally also avoid using regions but instead just use the entire method body. We can easily attribute the method with the name for the sample so that we can align them with the readmes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also by attribute we don't actually have to depend on a .NET Attribute type but we can simply add a structured comment on the method and look for that as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using the full method would definitely favor not using the full samples, lest our ReadMe
files become a novel. It could work in something isolated with a very intentional scoping to snippet use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DocFX uses regions that's why I went with them, it would be easy to reuse code when writing doc articles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome!
I have one suggestion about naming the regions, and some other feedback, but this is great!!
If we're using DocFX already, why not just use https://dotnet.github.io/docfx/spec/docfx_flavored_markdown.html#code-snippet? |
Very nice call out @heaths. That allows you to extract code via tag name, which is desirable over line numbers as those will likely change over time. |
We are not using line numbers here, we are using regions which is also what DocFX does |
Adds a tool that injects snippets from samples folder into README.md files.
The syntax is (remove space from ` ``):
To mark a snippet in you sample use
region
:After running the tool snippets would be updated directly in README.md replacing existing ones.
.\eng\Update-Snippets.ps1
Use
.\eng\Update-Snippets.ps1
to update snippets repo-wide