-
-
Notifications
You must be signed in to change notification settings - Fork 170
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 specific error for Show-Advice #287
Conversation
@robdy hmmm.... probably best if we sort out & merge the tests first and then you can make necessary changes to the tests in this PR, just to be safe. 💖 |
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.
Thanks for taking care of this! 💖
Couple of minor bits and pieces I'd like to iron out, and then we'll have to get the tests merged and updated a bit here, then we're good to go! 😊
Co-Authored-By: Joel Sallow (/u/ta11ow) <32407840+vexx32@users.noreply.github.com>
Co-Authored-By: Joel Sallow (/u/ta11ow) <32407840+vexx32@users.noreply.github.com>
Co-Authored-By: Joel Sallow (/u/ta11ow) <32407840+vexx32@users.noreply.github.com>
Co-Authored-By: Joel Sallow (/u/ta11ow) <32407840+vexx32@users.noreply.github.com>
@robdy are you still interested in working on this one? 🙂 (And if not / you're too busy, no worries, I can finish this specific issue up sometime this week and we can tackle something else later! 😊) |
@vexx32 let me commit what I have today evening, I have this pretty much ready |
Few comments to the recent changes. To generate incorrect files I had to adjust the existing tests. I tried to create new ones but it required loading them before. To avoid that, the test import data, modify it, perform tests and then restore the initial value. |
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.
Okay! If it works, it works. 😊
I'm wondering whether we could possibly bypass that by Mock
ing Get-Content
to just return the JSON string directly, for the purposes of that test? As long as you apply the Mock
within the PSKoans module scope it should work... I think. (But that's me mainly guessing, so I might be overlooking something!)
Otherwise this all looks good! Thank you so much! 💖
Perhaps we can open an issue for that and I can check if I'm able to rewrite this later as you described - what do you think? |
Yeah, we can roll with that. 👍 |
PR Summary
Fixes #286.
Context
As discovered in #285 and then noted in #286,
Show-Advice
should be more specific while erroring. I added two specific checks (if Advice was found at all and if title and content is not empty), each of them throwing specific errors. Initially I thought about having separate errors for missing title and missing content, but I think this should be handled in tests, not in the cmdlet definition (correct me if you think opposite.As #283 (tests for
*-Advice
) is not yet merged, I can open an issue to have it noted, that tests should be adjusted to verify that behavior. I can wait until it's merged and then modify the tests if that's more preferred.Changes
ObjectNotFound
error forShow-Advice
InvalidData
error forShow-Advice
Checklist