core, path: Refactor plugin path handling #131
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
I realized the `while true` loop from `_@go.run_plugin_command_script` could live comfortably in `_@go.set_search_paths` and handle the top-level `_GO_PLUGINS_PATHS` case as well. Also, I realized that there was no need for `@go` to invoke `_@go.run_plugin_command_script` if the command script in question was from the `_GO_SCRIPTS_DIR` of the current plugin and not in `_GO_SCRIPTS_DIR/plugins`. Plus, there was a bug whereby it'd been presumed that the plugin command path would always reside in the top-level `_GO_SCRIPTS_DIR` of the plugin. Now the plugin's `_GO_SCRIPTS_DIR` is correctly set to the `/bin` parent directory of the command script. I'll add thorough tests for this in a future commit/PR.
mbland
added a commit
that referenced
this pull request
Jan 25, 2017
While trying to create a test case for #131 and #132 to demonstrate that setting the `_GO_SCRIPTS_DIR` for a plugin incorrectly may cause it to find the wrong command scripts, the test exited with an error at some point because of the constraint that no duplicate scripts are allowed by `_@go.find_commands` (via `_@go.merge_scripts_into_list`). Since plugins may contain command names that match those in other plugins or the top-level `_GO_SCRIPTS_DIR`, and since `_GO_SEARCH_PATHS` has the correct semantics with regard to which command should take precedence for any given script, I realized raising an error was no longer appropriate. Now the first command found via searching `_GO_SEARCH_PATHS` takes precedence over duplicates in later paths.
mbland
added a commit
that referenced
this pull request
Jan 25, 2017
Closes #132. Adds test cases missing from #131. As demonstrated by the "plugin subcommand finds correct command in own plugin" test case, not setting `_GO_SCRIPTS_DIR` correctly for a plugin could've caused serious bugs: - showstopper (best case): no other command script of that name exists outside the plugin - obscure (medium case): finding the wrong script could cause confusing error messages when the interface between the caller and callee don't match - subtle (worst case): finding the wrong version of a script could exhibit confusing behaviors that might only become apparent after-the-fact
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
I realized the
while trueloop from_@go.run_plugin_command_scriptcould live comfortably in_@go.set_search_pathsand handle the top-level_GO_PLUGINS_PATHScase as well.Also, I realized that there was no need for
@goto invoke_@go.run_plugin_command_scriptif the command script in question was from the_GO_SCRIPTS_DIRof the current plugin and not in_GO_SCRIPTS_DIR/plugins.Plus, there was a bug whereby it'd been presumed that the plugin command path would always reside in the top-level
_GO_SCRIPTS_DIRof the plugin. Now the plugin's_GO_SCRIPTS_DIRis correctly set to the/binparent directory of the command script. I'll add thorough tests for this in a future commit/PR.