This repository has been archived by the owner on Sep 8, 2024. It is now read-only.
Add 'msm remove', improve error handling and parsing #1093
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Semi-major rework of the Mycroft Skill Manager (MSM), including:
* Add parse skill name strings support for quoted strings, e.g.:
msm install "daily meditation"
Previously it would search for "daily" and also search for
"meditation", but both independently.
* All commands return distinct error codes. Commands with
that perform multiple things, such as 'msm default' will
continue on a failure of one skill and return the highest
error code ultimately.
* Add 'msm remove', essentially the inverse of 'msm install'
* Changed the example install URL to a penrods repo since the
ethanward repo no longer exists.
* Reworked skill list caching to behave properly with $( )
subprocess operations.
* Output for "install" and "remove" is now between hyphen
lines, making them easier to parse.
* Revamped many informational and error messages
* Published under Apache 2.0 license
A corresponding update to the skill-installer will take
advantage of these changes.
penrods
added
CLA: Yes
penrods
added
tagged for review and potential merge
and removed
Status: Work in progress
forslund
suggested changes
Sep 18, 2017
msm/msm
Outdated
| echo "Installing requirements..." | ||
| if [[ "${picroft}" == 'false' ]]; then | ||
| echo " Installing requirements..." | ||
| if [[ "${picroft}" == "false" ]]; then |
There was a problem hiding this comment.
Something is weird here. On my desktop system the script entered the else branch of this if-statement and installed requirements using sudo
Debug output:
+ echo ' Installing requirements...'
Installing requirements...
+ [[ '' == \f\a\l\s\e ]]
+ sudo pip install -r requirements.txt
There was a problem hiding this comment.
I guess it's that the picroft variable has been renamed picroft_mk1
msm/msm
Outdated
| # due to ambiguity. | ||
| # | ||
| echo "Multiple matches for '${str}', be more specific." | ||
| echo "----------------------------------------------------------------------" |
msm/msm
Outdated
| deactivate mycroft | ||
| return 121 | ||
| fi | ||
| else | ||
| echo "ERROR: Unable to activate mycroft virtualenv!" | ||
| echo "ERROR: Unable to activate Mycroft virtualenv, requirements not installed." |
There was a problem hiding this comment.
Should this be small letters since the Mycroft virtualenv name is actually mycroft
| exit_code=$? | ||
| if [[ ${exit_code} -gt 0 ]] ; then | ||
| echo "${script}: error ${exit_code}" | ||
| exit ${exit_code} |
There was a problem hiding this comment.
These error printout/exit could be moved to a separate function since they're duplicated for each msm command
There was a problem hiding this comment.
I'm not clear on what you mean here
There was a problem hiding this comment.
Nothing really important but one could create a function wrapping the above lines since they exist a couple of times in the script.
function exit_error() {
echo "$1: error $2"
exit $2
}Cleaning up several small problems found wiht new MSM during review: * Message formatting * Incorrect spelling on a env var used
forslund
approved these changes
Sep 18, 2017
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Semi-major rework of the Mycroft Skill Manager (MSM), including:
msm install "daily meditation"
Previously it would search for "daily" and also search for
"meditation", but both independently.
that perform multiple things, such as 'msm default' will
continue on a failure of one skill and return the highest
error code ultimately.
ethanward repo no longer exists.
subprocess operations.
lines, making them easier to parse.
A corresponding update to the skill-installer will take
advantage of these changes.