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 skill] Internet Radio #143

Open
LazzaAU opened this issue Sep 11, 2022 · 2 comments
Open

[New skill] Internet Radio #143

LazzaAU opened this issue Sep 11, 2022 · 2 comments

Comments

@LazzaAU
Copy link

LazzaAU commented Sep 11, 2022

Dear Psycho, blablabla....

Ok, seriously, thank you for your work! Please fill the following informations so that we can review your skill and decide if it is in a publishable state. Do not remove the fields, but answer to them.

  • My skill name: InternetRadio

  • What does my skill do: Plays internet radio :)

  • The skill github repository url: https://github.com/LazzaAU/skill_InternetRadio

  • Did you check prior if there's already a skill existing that does the same?: Yes

    • If you answered no: DO IT and stop here. Return when you have checked
    • If you answered yes:
      • Is there a skill already existing?: Yes

      • If you answered yes:

        • Why, in your opinion, do you think we should publish yours?:

        well...... where do i start ? hehe.
        BRB just going to put on my salesmans hat,

        In todays society, peoples life can be crazily busy. With all this fuss and pressures of life who has time to navigate advanced radio station skills ? Can you picture yourself unwinding from a busy day at the office in the comfort of your pool side deck chair sipping on cocktails and asking alice to "play some jazz on the radio" and having those soothing sounds melt all your troubles away ? that's right ... simplicity and light weight is the key here , save the hassle of logging into mpd servers and scrolling playlists, JUST PLAY AND WALK AWAY :) and this week only... you get it for a special price of $0.00. Act now and i'll throw in some free ginzu knives and a pet rabbit called chuck. But wait thats not all... tell your friends and they too can get Internet Radio for free. THAT's a CRAZY deal :)

On a side note:
Apart from that I didn't realise Nepo had mode a advanced version until recently :) (which by the way doesn't show in the store)

- If you answered no:
  - Great, let's continue!
  • Did you make sure your skill follows our code guidelines?: Yup

    • If you answered yes: Perfect, thank you!, your welcome :)
    • If you answered no: Well, no chance at publishing buddy.... Code guidelines
  • Did you successfully run projectalice-sk validate on your skill? yes

    • If you answered yes: Very well, you can continue!
    • If you answered no: Really? Publishing a skill
  • Do you believe in zombies? Yes/No/Seriously?? i've answered this one enough times now :)

  • If you answered seriously??:

    • Do I look like I'm kidding?

Ok, so you've gone through these steps, you can now submit your request. We'll fork your repository, make sure everything is in order and if yes we'll publish the skill. If anything is not ok, we'll get back in touch with you to get your skill published as soon as possible. Don't forget that once your skill is forked, you'll need to make Pull Requests to update your skill which will need the same approval.

@philipp2310
Copy link
Member

philipp2310 commented Sep 26, 2022

I'm currently translating to german and reviewing in the progress, to prevent double work, I fix the small things right away and just document it for future work:

Talk/Dialog Files:

  • Message names should be speaking: dialogMessageX => NrOutOfRange/StationNotFound/Confirmation/StartPlaying/StopPlaying
  • The "short" message should still be informative enough, as it can be a default, not only "night" option: "Failed" => "Number too big"/"Try below {1}"
  • Why should the user edit the config.json.template?? That won't allow updates as the skill is now modified. I think it should be some kind of real config - or you request the users to create a PR with their stations so they can return back to the latest version of the skill.
  • When creating backups - how about moving them out of the skills folder to for example a dedicated backup folder in ProjectAlice/var or ProjectAlice/system. Creating a backup inside the skills folder can prevent updates (or delete the backup on trying to fix the repo..)
  • Why use a slot for "StopTheRadio" that is only a fixed value of "Stop The Radio". As it is optional you might end up with a "StopPlayingRadio" intent, but unfilled slot. And now? It is quite hard to translate as well! "{stop the radio} playing the radio" would be a "valid" input for your training
  • use the "icon" category in the *.install -> "fas fa-radio"
  • is online really a 100% requirement? The skill won't run when I got my own LAN radio station with this setting (edge case...)

Coding

  • Avoid redundancy: _CONFIGTEMPLATE could be reused for example in _templatePath = f'/InternetRadio/{_CONFIGTEMPLATE}'. Same for BackupConfigTemplate.
  • line 37: double checked for stop: Intent is Stop, checked again for slot. Without the slot it only says "ok I stopped" but won't do it!
  • Speaking Talk Names: Line 45 was switched to "StartPlaying" instead of "StopPlaying" because the number DialogMessage4 didn't say "I'm wrong here" as the name would have done
  • the order of a dictionary is not guaranteed by python, looping and getting number X COULD break

What I learned:

  • We need some dynamic slot population from skills - like you did manually - good job! ^^

@LazzaAU
Copy link
Author

LazzaAU commented Sep 26, 2022

  • Why should the user edit the config.json.template??

    • good point. It's done that way because i couldn't think of another way for the user to "select" a radio station. I did transfer it to a stations.json file at one stage but that proved to be annoying because it required Alice to list the stations to choose from in the systemlog or announce them and then have the user choose verbally kinda thing.
  • The backup folder is ignored in .gitignore and is not a true automatic backup, as per instructions it was more so a added bonus to triggering the automatic adding of users station names to the dialogFile. But yes thanks, never thought about using ProjectAlice/var or ProjectAlice/system.

  • Message names should be speaking: dialogMessageX =

    • yep, have corrected that in my automatic talkCreator script since the making of this skill
  • online only ?

  • i'm not familiar with LAN radio stations so never considered that, My thinking was to stream from internet means needing internet. point noted

Thanks for the feedback :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants