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

(GH-125) Make Header ID optional #127

Merged
merged 1 commit into from
Oct 21, 2020

Conversation

glennsarti
Copy link
Contributor

Fixes #125

Previously the Id parameter to New-BTHeader was mandatory however as it can be
a generated unique string it doesn't have to be. This commit modifies the
Id parameter so it is no longer mandatory and uses a GUID style string as the
autogenerated Id if none is passed. GUIDs are used instead of, say, the title
because they are guaranteed to be unique whereas using the title or date/time
could cause notification Id collisions.

This commit also adds a test to ensure that the function can be called without
the -Id parameter and that it is actually generated, not left as null or an
empty string.

Previously the Id parameter to New-BTHeader was mandatory however as it can be
a generated unique string it doesn't have to be.  This commit modifies the
Id parameter so it is no longer mandatory and uses a GUID style string as the
autogenerated Id if none is passed.  GUIDs are used instead of, say, the title
because they are guaranteed to be unique whereas using the title or date/time
could cause notification Id collisions.

This commit also adds a test to ensure that the function can be called without
the -Id parameter and that it is actually generated, not left as null or an
empty string.
@codecov
Copy link

codecov bot commented Oct 8, 2020

Codecov Report

Merging #127 into v0.8.4 will increase coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           v0.8.4     #127      +/-   ##
==========================================
+ Coverage   59.29%   59.38%   +0.08%     
==========================================
  Files           1        2       +1     
  Lines         457      916     +459     
  Branches        3        6       +3     
==========================================
+ Hits          271      544     +273     
- Misses        183      366     +183     
- Partials        3        6       +3     
Impacted Files Coverage Δ
D:/a/1/s/BurntToast/BurntToast.psm1 59.38% <0.00%> (ø)
BurntToast/BurntToast.psm1 59.38% <0.00%> (+0.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 52115dc...06f9119. Read the comment docs.

@Windos
Copy link
Owner

Windos commented Oct 11, 2020

Awesome, thanks @glennsarti!

Just to talk through this a bit... I think in this case ID collisions are expected (and for most users that aren't supplying their own, desired).

I might be wrong on this point so keen on bounce the use cases around.

This works great if you're creating one header object and re-using it inside one script:

$Id = $Id = 'ID' + (New-Guid).ToString().Replace('-','').ToUpper()
$Header = New-BTHeader -Title 'Example Header' -Id $Id

New-BurntToastNotification -Header $Header -Text 'Toast 1'
New-BurntToastNotification -Header $Header -Text 'Toast 2'
New-BurntToastNotification -Header $Header -Text 'Toast 3'

This will result in the toasts appearing under the same header as expected.

Single header

However, if it's a one toast per script run and the script is run multiple times, the toasts will show under different headings:

# Script run 1

$Id = $Id = 'ID' + (New-Guid).ToString().Replace('-','').ToUpper()
$Header = New-BTHeader -Title 'Example Header' -Id $Id

New-BurntToastNotification -Header $Header -Text 'Toast 1'

# Script run 2

$Id = $Id = 'ID' + (New-Guid).ToString().Replace('-','').ToUpper()
$Header = New-BTHeader -Title 'Example Header' -Id $Id

New-BurntToastNotification -Header $Header -Text 'Toast 2'

# Script run 3

$Id = $Id = 'ID' + (New-Guid).ToString().Replace('-','').ToUpper()
$Header = New-BTHeader -Title 'Example Header' -Id $Id

New-BurntToastNotification -Header $Header -Text 'Toast 3'

Different headings


I think in that second case a "regular" user would expect the headings to match (given that all they would be supplying after this PR is the title and the ID will be invisible to them).

Maybe worth a poll?

@Windos Windos added enhancement HACKTOBERFEST A good target for #Hacktoberfest PRs. Learn, contribute, earn a shirt! hacktoberfest-accepted labels Oct 19, 2020
@glennsarti
Copy link
Contributor Author

I have the opposite view but that's mainly due to New-BTHeader. That should be creating a new resource, not re-using an existing one. So in the absence of specific information, that is having to guess an ID, it should defer to making a new header

I'd also argue that what I wrote actually preserves the existing script behaviour where you'd have to know the Header ID across script invocations.

@glennsarti
Copy link
Contributor Author

glennsarti commented Oct 21, 2020

Managing state across script invocations is tricky. Currently that burden is on the user of the module (because the Id is Mandatory).

Perhaps an additional helper method is needed Get-BTHeader -Title "something" ?

Which would go through the Toast History looking for a toast notification with a header title of 'something' and then return that header object ?

So an example script ...

$Header = Get-BTHeader -Title 'Example Header'
if ($Header -eq $null) { $Header = New-BTHeader -Title 'Example Header' }

New-BurntToastNotification -Header $Header -Text 'Toast 3'

@Windos Windos changed the base branch from main to v0.8.4 October 21, 2020 08:29
@Windos
Copy link
Owner

Windos commented Oct 21, 2020

@glennsarti having mulled it over a bit, I think you're 100% correct.

If maintaining heading id is important, it should be hardcoded in the calling script (or reliably generate) as it would have to be today anyway.

The helper function you mentioned would be a good addition too, and I've spun that comment off into an issue.

@Windos Windos merged commit 0f3e32e into Windos:v0.8.4 Oct 21, 2020
@glennsarti
Copy link
Contributor Author

Thanks @Windos !!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement HACKTOBERFEST A good target for #Hacktoberfest PRs. Learn, contribute, earn a shirt! hacktoberfest-accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Header ID shouldn't be mandatory!
2 participants