New plugin version selection implementation #363
Conversation
|
Hey @riknoll, Thanks for doing this! I'll review it after the cordova 6 release. |
| } | ||
| } | ||
| } | ||
|
|
||
| // Fetch the plugin first. | ||
| events.emit('verbose', 'Calling plugman.fetch on plugin "' + target + '"'); |
There was a problem hiding this comment.
Might target be undefined here?
There was a problem hiding this comment.
Yup, this line should have been moved down with the rest
|
@stevengill just checking in now that Cordova 6.0.0 is released. Let me know if you have any feedback. |
|
Rebased to master and responded to PR feedback. @TimBarham @vladimir-kotikov can you take a look at this as well? |
| @@ -492,17 +492,13 @@ function list(hooksRunner, projectRoot, opts) { | |||
| var platforms_on_fs = cordova_util.listPlatforms(projectRoot); | |||
| return hooksRunner.fire('before_platform_ls', opts) | |||
There was a problem hiding this comment.
You don't need platforms_on_fs anymore, since you get equivalent information from your call to getInstalledPlatformsWithVersions() (so you can use platformMap below instead of platforms_on_fs).
|
@TimBarham thanks for the review; I'll update the PR in a bit! |
|
Responded to most the feedback. Also added warnings and verbose logging which I completely forgot in my original PR. I will rebase this branch soon |
|
Rebased to master (that was fun) |
| it('clean up after plugin fetch spec', function() { | ||
| removeTestProject(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This is really pretty code.
|
@TimBarham addressed your other feedback |
| .then(function(v) { | ||
| result[p] = v || null; | ||
| }, function(v) { | ||
| result[p] = v || 'broken'; |
There was a problem hiding this comment.
Is this what you intended? The parameter v here will actually be the error. In the previous version, v was simply ignored (so "version" was always set to "broken").
|
@vladimir-kotikov to also take a look. |
|
I have significant changes to push in response to some feedback, so don't review yet. I'll comment when they're in. |
|
Alright, this should be ready for review. The changes I just pushed included a few fixes to edge cases, better handling of malformed input/whitespace, and a lot more tests. |
|
@TimBarham @vladimir-kotikov Can you please take a look in the next couple of days? It will be good to get this committed and do a release. |
| var version = parts[1]; | ||
|
|
||
| // If no version is specified, retrieve the version (or source) from config.xml | ||
| if (!version && !cordova_util.isUrl(id) && !cordova_util.isDirectory(id)) { |
There was a problem hiding this comment.
I'd rather reverse if condition here to return target immediately if there is no need for any processing. This is just a matter of taste of course but IMO it would help to improve code readability a bit. The same for L302 and L304
There was a problem hiding this comment.
Right, I just refactored this code, but I should have caught that. Changing now.
|
LGTM apart from a couple of nitpicks. |
| } | ||
|
|
||
| listUnmetRequirements(latestFailedReqs); | ||
| events.emit('warn', 'Current project does not satisfy the engine requirements specified by any version of this plugin. Fetching latest or pinned version of plugin anyway (may be incompatible)'); |
|
LGTM! Great work @riknoll! Very clean code and is easy to follow. Looking forward to switching over to this. |
| // If no version is specified, retrieve the version (or source) from config.xml | ||
| if (version || cordova_util.isUrl(id) || cordova_util.isDirectory(id)) { | ||
| return Q(target); | ||
| } else { |
There was a problem hiding this comment.
Minor nit: superfluous else - remove it, and decrease the indentation level of the rest of the function.
|
This is looking great @rikroll! I have a few comments, but mostly pretty minor stuff. |
|
Okay, I added a lot of verbose logging and responded to some of the refactor stuff @TimBarham mentioned (except where noted). I plan to rebase this down to one commit and merge at end of day today. |
|
@TimBarham updated |
@stevengill @dblotsky please review. This is an implementation for the plugin version selection scheme that Dmitry and I proposed in this discuss PR. I'd appreciate some feedback! I have some docs written up for this too, I'm just holding off on that PR just yet.