Tyrrrz / Ressy

Resource editor for PE files
MIT License
56 stars 8 forks source link

Replace readme examples with actual code snippets #3

Closed SimonCropp closed 2 years ago

SimonCropp commented 2 years ago

I also used Verify to generate the output of the snippets.

Tyrrrz commented 2 years ago

Hi and thanks a lot for the PR :)

Some questions:

  1. Would I be right to assume that the ultimate goal of this tool is to just have all code snippets encoded as actual tests? Meaning that instead of Snippets.cs I could just reference existing test code (given that it has all the samples, of course) in readme?
  2. Are #region blocks required for identification? Or can I also reference a snippet by method name?
  3. Are the code snippet links in readme updated automatically by GitHub actions? Specifically I'm interested in the hrefs like this one <a href='/Ressy.Tests/Snippets.cs#L26-L32' title='Snippet source file'>snippet source</a>

Overall I think MarkdownSnippets provides a nice way to ensure that code samples stay in-sync with API or behavioral changes. On the other hand, I'm not 100% convinced that the added overhead (albeit small) is worth it, given that I don't really have too many issue maintaining code samples (it's the human text part that's really hard 😅). I'll give it some more thought later. 🤔

Also, I wonder if GitHub allowed to put permalinks to code in markdown files in the same way it does in issues/PRs (where it renders that code in-place), then this could be simplified. 🤔

SimonCropp commented 2 years ago

Would I be right to assume that the ultimate goal of this tool is to just have all code snippets encoded as actual tests?

Not always. Often tests make for good snippets, but not always. The requirements of a test (useful, fast and readable for the maintainer) can often conflict with the requirement of a snippet (readable for a consumer, concise, explains a specific api/scenario). But yes, sometimes you can use tests for snippets. You can also sometimes use implementation code as snippets, eg to explain a behavior of a scenario. Note that mdsnippets scans the whole solution directory

Are #region blocks required for identification?

No Comments are also supported. The value of regions is that is forces a beginning and end that is verified by the compiler. So you get a more useful error than the one you get from the mdsnippets engine when it detects a missing end.

If you dont like the collapsing behavior of regions you can disable it. TBH i would recommend disabling it anyway so in any code base you open they will expand. I know many people hate on regions, but when you change the default behavior to "they are expanded", most of the reasons for that hate are mitigated.

Or can I also reference a snippet by method name?

No this is not currently supported. It is theoretically possible to do with roslyn, but the effort and likely perf impact has meant i have not gone down that route.

Are the code snippet links in readme updated automatically by GitHub actions? Specifically I'm interested in the hrefs like this one

No these are generated by mdsnippets. The addition GH action in this pr performs the same snippet+md merging that occurs when you build. the reason i also use a GH action is so that if you edit some code online or push without rebuilding tests, then the snippets will still merged into the md files

I'm not 100% convinced that the added overhead (albeit small) is worth it

Yep understood. Dont feel abliged to accept this PR. I wont be offended if you dont want to take this approach

given that I don't really have too many issue maintaining code samples (it's the human text part that's really hard 😅)

Well you did have one bug in your snippets using var iconFileStream = File.Open("new_icon.ico"); does not compile 😄

Tyrrrz commented 2 years ago

Thanks for the explanation! It all makes much more sense now.

The addition GH action in this pr performs the same snippet+md merging that occurs when you build.

Aaahh I see. So technically it's not required, it's just useful to have. Makes sense. I should probably just the readme, I'm sure most of my questions are answered there. 😅

Yep understood. Dont feel abliged to accept this PR. I wont be offended if you dont want to take this approach

I still see some definite benefits with this, so maybe I just need some time to reconcile it in my head. For now I'll just leave this PR around and see if I come back to it eventually.

Thanks a lot for showing me mdsnippets 🙂

Well you did have one bug in your snippets using var iconFileStream = File.Open("new_icon.ico"); does not compile 😄

😱 Damn, the one thing I didn't verify 😂

Tyrrrz commented 2 years ago

Decided to leave it as is for now, but thank you for the introduction to Verify and MarkdownSnippets. I may come back to it when I figure out how to better organize examples with the rest of the code (personal OCD issues 😅).