Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Make disabledDirectory and apiVersion required for extension installation #3183

Merged
merged 3 commits into from
Mar 21, 2013

Conversation

dangoor
Copy link
Contributor

@dangoor dangoor commented Mar 20, 2013

For #3159, addressing comments that disabledVersion and apiVersion should be required for the extension installation function.

@ghost ghost assigned peterflynn Mar 20, 2013
@dangoor
Copy link
Contributor Author

dangoor commented Mar 20, 2013

To @peterflynn

@@ -332,10 +332,14 @@ function _removeAndInstall(packagePath, installDirectory, validationResult, call
*
* @param {string} Absolute path to the package zip file
* @param {string} the destination directory
* @param {{disabledDirectory:string}} additional settings to control the installation
* @param {{disabledDirectory:string, apiVersion:string}} required additional settings to control the installation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add the optional nameHint property here too for me? Sorry about the oversight.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the required params, we should change the "?"s to "!"s in the JSON docs below too.

@peterflynn
Copy link
Member

@dangoor: done reviewing. Just a few nits.

@dangoor
Copy link
Contributor Author

dangoor commented Mar 21, 2013

OK, comments addressed (adjusted the comments to reflect required/optional status, made two tests that check that apiVersion and disabledDirectory are both required)

@dangoor
Copy link
Contributor Author

dangoor commented Mar 21, 2013

Travis is happy and the comments were nits, so I'm going to go ahead and merge.

dangoor added a commit that referenced this pull request Mar 21, 2013
Make disabledDirectory and apiVersion required for extension installation
@dangoor dangoor merged commit 51ec333 into master Mar 21, 2013
@dangoor dangoor deleted the dangoor/required-install-fields branch March 21, 2013 01:10
@peterflynn
Copy link
Member

Seems reasonable to me -- thanks

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

Successfully merging this pull request may close these issues.

2 participants