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

Add tmdb-skill #890

Closed
wants to merge 1 commit into from
Closed

Conversation

builderjer
Copy link
Contributor

@builderjer builderjer commented Feb 24, 2019

Info

This PR adds the new skill, tmdb-skill, to the skills repo.

Description

Uses the TMDb api to call information about movies, actors, production companies and the like.

Created with mycroft-skills-kit v0.3.12

@builderjer
Copy link
Contributor Author

I read the checks, and I'm not sure why it failed. Can you explain?

@krisgesling
Copy link
Contributor

It appears to be problem with the alarm skills test suite. We've been having a little trouble with it lately. Let me check it out properly tomorrow and I'll let you know if there's actually a problem.

@krisgesling
Copy link
Contributor

If anyone else wants to look into it in the meantime, 008.delete.alarm.json appears to be returning dialog to cancel 1 alarm, when there should be 2 alarms still set.

@builderjer
Copy link
Contributor Author

I have submitted a couple of commits to this skill, mainly to clean up the code. Everything but line length is pep8 compatibale now, but I still don't see any errors.

I'm not sure I put them where the comment says, but I thought I did. If not, the latest code is in the main repo

@krisgesling
Copy link
Contributor

Hey, to get those latest commits into this PR, just run
mycroft-msk upload /opt/mycroft/skills/your-skill

This will also re-run the tests.

No worries about the PEP8 line length either.

@builderjer
Copy link
Contributor Author

It looks like this skill passed the tests this time!!

I would love to get some feedback on this. Maybe some code streamlining?

@krisgesling krisgesling added new New Skill (not update to existing Skill) needs validation Needs validatiion by another Skills Team Member or Community Member trying it labels Mar 2, 2019
@builderjer builderjer force-pushed the add-tmdb-skill branch 2 times, most recently from b846b7c to 4d27536 Compare March 3, 2019 22:42
@builderjer
Copy link
Contributor Author

I did a bug fix, and now it is not passing the tests. Once again, I don't think it is this skill, but the alarm skill

@builderjer builderjer force-pushed the add-tmdb-skill branch 2 times, most recently from 5512308 to 2fe6246 Compare March 8, 2019 00:45
@builderjer
Copy link
Contributor Author

Everything has passed, it would be great to get some feedback on this skill. @andlo , I added a couple more vocab and dialog files maybe you could translate for me and do a PR for them? That would be great!!

@builderjer builderjer force-pushed the add-tmdb-skill branch 3 times, most recently from d327943 to 256b9df Compare March 11, 2019 00:44
@forslund
Copy link
Collaborator

Run test

@builderjer
Copy link
Contributor Author

Nice!! Everything passed!!

@krisgesling
Copy link
Contributor

Closing PR on old branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs validation Needs validatiion by another Skills Team Member or Community Member trying it new New Skill (not update to existing Skill)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants