Skip to content
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

New-MDAlert commandlet and its Tests #37

Merged
merged 9 commits into from
Jun 6, 2024
Merged

Conversation

belibug
Copy link
Contributor

@belibug belibug commented May 31, 2024

Aligns with and depends on New-MDQuote commandlet internally.

Aligns with and depends on New-MDQuote commandlet internally.
@AppVeyorBot
Copy link

Build MarkdownPS 1.0.38 completed (commit 9cc7140f37 by @belibug)

@belibug
Copy link
Contributor Author

belibug commented May 31, 2024

Let me know if it meets the general standards.

@Sarafian
Copy link
Owner

Sarafian commented Jun 4, 2024

GH-28

@Sarafian
Copy link
Owner

Sarafian commented Jun 4, 2024

The concept is correct. Some remarks:

  • The examples in the description are not correct. It should be more explicit that this is GFM with a link in the description. This would explain what the different option mean for Style as well
  • Note that an alert can't have "level 2 quote".
  • Please note that I only use UpperCase variable names for parameters. Internal variables are camelCase which is probably not very PowerShell-ly. You can look in the rest of the code
  • In case you pipe this cmdlet the Begin section won't work correctly as it will be called only once. It wasn't meant for this function anyhow. So I would had done something like $Admonition+$Lines when calling the New-MDQuote once which would also make tests easier. Look next one.
  • The tests are not checking if the MD-Quote is called correctly. Not sure which is the best way in this simple case. Check the output or what happened. I would go with what happened, just in case the New-MDQuote is ever changed. So test with mocks.
  • We need to add an example in the Demo.ps1 and adapt the README.md as well with the code and the rendering
  • Add the new cmdlet in the CHANGELOG.md

Don't get me wrong, I could had done this already but you asked me to "learn" and I can't contribute in the pull or at least I don't know how any longer :)

@belibug
Copy link
Contributor Author

belibug commented Jun 4, 2024

The examples in the description are not correct. It should be more explicit that this is GFM with a link in the description. This would explain what the different option mean for Style as well

Noted, will address this.

Note that an alert can't have "level 2 quote".

Yes, that’s why I removed the level parameter, so its always level 1

Please note that I only use UpperCase variable names for parameters. Internal variables are camelCase which is probably not very PowerShell-ly. You can look in the rest of the code

Noted, will double check this

In case you pipe this cmdlet the Begin section won't work correctly as it will be called only once. It wasn't meant for this function anyhow. So I would had done something like $Admonition+$Lines when calling the New-MDQuote once which would also make tests easier. Look next one.

This is intentional, we want > [!TIP] tag applied only once when data is sent from pipeline, which it does when begin block runs. When there are 2 or more lines from pipeline this will work as per requirement. I could move it to process block and handle it differently, but that would complicate the flow. I am open for suggestion here.

The tests are not checking if the MD-Quote is called correctly. Not sure which is the best way in this simple case. Check the output or what happened. I would go with what happened, just in case the New-MDQuote is ever changed. So test with mocks.

Generally I write tests which are agnostic of function logic. All tests are are “expected output” for a “specific input” pattern. If in case New-MDQuote is ever changed, it would instantly break any (all?) tests related to MDAlret because it depends on it internally, thus serving tests purpose. I could add mock, but you already have added extensive tests for MDQuote so it felt redundant.

We need to add an example in the Demo.ps1 and adapt the README.md as well with the code and the rendering
Add the new cmdlet in the CHANGELOG.md

This needs to handled, I did not know how much is too much for a single commit. I will add section in Demo.ps1 and update README accordingly. Do you prefer same pull request or a new one, I have seen others frown when a single pull request does too much changes.

Note

I really appreciate this opportunity, your feedback is greatly welcome.

1. Now includes reference to GFM link
2. Detailed description about the function and link to GFM styles
3. Sytle parameter in help shows possible input option
4. Change local variables to match module naming convention
@AppVeyorBot
Copy link

Build MarkdownPS 1.0.39 completed (commit bb38453b05 by @belibug)

@belibug
Copy link
Contributor Author

belibug commented Jun 4, 2024

  1. Now includes reference to GFM link
  2. Detailed description about the function and link to GFM styles
  3. Style parameter in help shows accepted values
  4. Change local variables to match module naming convention

@belibug
Copy link
Contributor Author

belibug commented Jun 4, 2024

Meanwhile, i am working on Demo.ps1 and its output in README.md file.

@AppVeyorBot
Copy link

Build MarkdownPS 1.0.40 completed (commit c9e8edc4d1 by @belibug)

@AppVeyorBot
Copy link

Build MarkdownPS 1.0.41 completed (commit b74e6d039e by @belibug)

@Sarafian
Copy link
Owner

Sarafian commented Jun 6, 2024

This is intentional, we want > [!TIP] tag applied only once when data is sent from pipeline, which it does when begin block runs. When there are 2 or more lines from pipeline this will work as per requirement. I could move it to process block and handle it differently, but that would complicate the flow. I am open for suggestion here.

This is a design choice which will probably never ever bother everyone. The way the module is coded is that if you ever pipelined it, it would render all the body everytime. The only thing that happens in begin is setup some constant variables like eg. $prefix="> " etc. So for consistency, it needs to go to the processblock. Also, the-NoNewLineparameter would not make sense if it was pipelines like this. But nobody would ever use it like this. I didn't when I made this and I had a lot of opportunities. The reason is in my opinion because you will need to add some text in between, so theForEach-Object` would make much more sense.

Generally I write tests which are agnostic of function logic. All tests are are “expected output” for a “specific input” pattern. If in case New-MDQuote is ever changed, it would instantly break any (all?) tests related to MDAlret because it depends on it internally, thus serving tests purpose. I could add mock, but you already have added extensive tests for MDQuote so it felt redundant.

OKAY. Agreed. Lets keep it like this.

@Sarafian
Copy link
Owner

Sarafian commented Jun 6, 2024

Some minors

  • There is a small mistake in one of the example were double > are shown.
  • In the demo and readme, I think it's better to use what github offers as text. Since there is a link for this, it's more appealing.
  • In the changelog, I only mention funcional assets. I copy this in the release field of the module. Mentioning Pester is not functional.
  • The date in the changelog needs to be roll back. I also didn't remember what was supposed to happen. (I need the contribution for myself :) ). In essense, when it will ready for release, depending on the change, I'll adapt the next version number and add a date. Manually that is.

belibug added 3 commits June 6, 2024 10:58
Removed non-functional changes mentioned in CHANGELOG
Fixed typo in example section of MDAlert
Removed non-functional changes mentioned in CHANGELOG
@AppVeyorBot
Copy link

Build MarkdownPS 1.0.45 completed (commit fa7a8d6b3d by @belibug)

@AppVeyorBot
Copy link

Build MarkdownPS 1.0.46 completed (commit 548e37e7c8 by @belibug)

Fixed typo in example demo.ps1 and corresponding README entry
@AppVeyorBot
Copy link

Build MarkdownPS 1.0.47 completed (commit c32e82f06a by @belibug)

@AppVeyorBot
Copy link

Build MarkdownPS 1.0.50 completed (commit 007922ce18 by @belibug)

@Sarafian Sarafian merged commit c1e047e into Sarafian:master Jun 6, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants