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

Minor improvements to the msm tool #1201

Merged
merged 6 commits into from Nov 8, 2017

Conversation

@theCalcaholic
Copy link
Contributor

theCalcaholic commented Nov 7, 2017

This pull requests adds two improvements to the msm command line tool.

  1. msm list will now show which skills are already installed (issue #1202)

  2. A new msm info <skill> command has been added, with either being a distinct substring of a skill's name or the skill's github repository's url. It will display the readme.md from the repository in the terminal (if found). (issue #1203)

@theCalcaholic
Copy link
Contributor Author

theCalcaholic commented Nov 7, 2017

Sorry for not referencing the issues in the commit messages. Next time I'll read the contribution guidelines before making the PR.

@forslund
Copy link
Member

forslund commented Nov 7, 2017

Thank you, this sounds like very useful features. I'll give it a go in a bit.

In the meantime I'd like you to sign the contributors licensing agreement (https://mycroft.ai/cla/), this is needed to make sure everything is Ok on the licensing side and you are aware what rights you're granting us.

@theCalcaholic
Copy link
Contributor Author

theCalcaholic commented Nov 7, 2017

Thanks for the information. I just signed the agreement. 👍

@penrods penrods added the CLA: Yes label Nov 7, 2017
@LearnedVector
Copy link
Contributor

LearnedVector commented Nov 8, 2017

This works really nicely. Tested and works for me. @forslund let me know if you have any concerns with this. otherwise it looks good to me. thanks @theCalcaholic!

@forslund
Copy link
Member

forslund commented Nov 8, 2017

I tested it earlier and it works really well. Merging. Big thanks @theCalcaholic!

@forslund forslund merged commit 363372c into MycroftAI:dev Nov 8, 2017
3 checks passed
3 checks passed
codacy/pr Good work! A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 46.137%
Details
@theCalcaholic
Copy link
Contributor Author

theCalcaholic commented Nov 8, 2017

I'm glad it works.

I think I should point out a short coming off the msm info command. It gets the information from the latest version's Readme instead of the version which is included in https://github.com/MycroftAI/mycroft-skills because I have no idea how to get the exact submodule version information without cloning the parent repository.

@forslund
Copy link
Member

forslund commented Nov 8, 2017

I think we still pull the head version so that's quite accurate. I'll make a note to check it over when we start to use the version the submodule is pointing to

@theCalcaholic
Copy link
Contributor Author

theCalcaholic commented Nov 8, 2017

@forslund Oh, perfect. Thanks!

@theCalcaholic theCalcaholic deleted the theCalcaholic:feature/msm-improvements branch Nov 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.