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

Replace API endpoint and speech synthesizer #418

Merged
merged 3 commits into from
Oct 19, 2020
Merged

Replace API endpoint and speech synthesizer #418

merged 3 commits into from
Oct 19, 2020

Conversation

johnheusinger
Copy link
Contributor

PR Summary

I noticed that the 'Fun with Cat Facts' advice snippet referenced a REST API that is no longer available. I found a replacement API and made some adjustments so that the result matches the original intention.

I also noticed that the example makes use of System.Speech which works on Windows PowerShell but not PowerShell Core. I switched to the SAPI.SpVoice COM object which is also compatible with PS Core.

Context

Relates to #260

Changes

  • Replaced unavailable API endpoint with a working one
  • Replaced System.Speech with SAPI.SpVoice for PS Core compatibility
  • Spelling and formatting corrections

Checklist

  • Pull Request has a meaningful title.
  • Summarised changes.
  • Pull Request is ready to merge & is not WIP.
  • Added tests / only testable interactively.
    • Make sure you add a new test if old tests do not effectively test the code changed.
  • Added documentation / opened issue to track adding documentation at a later date.

Copy link
Owner

@vexx32 vexx32 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me apart from one or two small things to adjust.

Thanks for sorting this out! 💖 😊

Comment on lines 15 to 18
"\t$SpeechSynth = New-Object -ComObject SAPI.SpVoice",
"\t$URI = 'https://catfact.ninja/fact'",
"\t$CatFact = Invoke-WebRequest -Uri $URI | ConvertFrom-Json | Select-Object -ExpandProperty fact",
"\t$SpeechSynth.Speak(\"Did you know? $Catfact\") | Out-Null",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johnheusinger can you add a note into either the code segment (as a comment) or into the above description mentioning this is Windows-only?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 🙂

"\t$SpeechSynth.Speak($CatFact.details)",
"\t$SpeechSynth = New-Object -ComObject SAPI.SpVoice",
"\t$URI = 'https://catfact.ninja/fact'",
"\t$CatFact = Invoke-WebRequest -Uri $URI | ConvertFrom-Json | Select-Object -ExpandProperty fact",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"\t$CatFact = Invoke-WebRequest -Uri $URI | ConvertFrom-Json | Select-Object -ExpandProperty fact",
"\t$CatFact = Invoke-RestMethod -Uri $URI | Select-Object -ExpandProperty fact",

IRM does a direct JSON conversion for this purpose 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And done. Thanks 🙂

@vexx32 vexx32 added Category-Advice 🗨️ Related to Advice cmdlets or library hacktoberfest-accepted PRs that have been accepted for Hacktoberfest PR-Awaiting-Author ✏️ Waiting on a response from the user who submitted the PR. labels Oct 18, 2020
@vexx32 vexx32 merged commit 54cf90e into vexx32:main Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category-Advice 🗨️ Related to Advice cmdlets or library hacktoberfest-accepted PRs that have been accepted for Hacktoberfest PR-Awaiting-Author ✏️ Waiting on a response from the user who submitted the PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants