Deprecate 'cordova plugin search' command #671
Conversation
- Remove cordova plugin search command - Remove search command message - Remove search command tests
Codecov Report
@@ Coverage Diff @@
## master #671 +/- ##
=========================================
- Coverage 80.68% 80.58% -0.1%
=========================================
Files 52 51 -1
Lines 2930 2916 -14
=========================================
- Hits 2364 2350 -14
Misses 566 566
Continue to review full report at Codecov.
|
erisu
added this to Pending Review
in Apache Cordova: Next Release Planning and Tracking
erisu
moved this from Pending Review
to Pending Release
in Apache Cordova: Next Release Planning and Tracking
erisu
moved this from Pending Release
to Pending Review
in Apache Cordova: Next Release Planning and Tracking
There was a problem hiding this comment.
I think the title of this PR is misleading. This does not deprecate cordova plugin search, it removes it.
Deprecating a feature would keep it as is, but add warning messages or documentation about its impending removal. At least that's my understanding of the terminology. Please do correct me if I'm wrong!
That being said, I don't see any use for this feature. And since it's strictly a convenience feature, I think it is OK to remove it without prior deprecation.
However, the squashed commit message should read
Remove 'cordova plugin search' command
| @@ -47,7 +47,7 @@ module.exports.listUnmetRequirements = listUnmetRequirements; | |||
|
|
|||
| function add (projectRoot, hooksRunner, opts) { | |||
| if (!opts.plugins || !opts.plugins.length) { | |||
| return Q.reject(new CordovaError('No plugin specified. Please specify a plugin to add. See `' + cordova_util.binname + ' plugin search`.')); | |||
| return Q.reject(new CordovaError('No plugin specified. Please specify a plugin to add.')); | |||
There was a problem hiding this comment.
Do we want to link to https://cordova.apache.org/plugins/ instead?
There was a problem hiding this comment.
@raphinesse I am OK with a link, but truthfully though CLI, I know that if condition is actually never reached. I was debating on deleting it but would rather have a refactor cleanup PR.
Though CLI, if a plugin is not provided, THIS is the actual if condition that is caught and throws a different reject message.
cordova-lib/src/cordova/plugin/index.js
Line 62 in ea181ad
We could update this condition instead, except it is also a generic message for both add and rm. It also does not support remove. Just more things being uncovered as I been looking at this originally.
There was a problem hiding this comment.
Ah, I see. Given these options, I think I'll got with the refactor cleanup PR 😄
My plan was to do a complete refactor of the CLI using yargs or something like that. Had no time to tackle that yet, though.
|
@raphinesse Yes, to deprecate or deprecated does means it would still exist, still work with the current version, possibly replaced with something newer and more preferred, and will be removed in the future. I might have gone a bit too far and just happened to have removed it when I noticed it only opens the URL and with the plugin append to the query string if passed. Kinda feels like an anti-cli pattern where it navigates a user away from CLI. Anyways, If you and others do prefer to deprecate and display a warning message that it will be removed in a future release, I can revert all the changes and add a single That would mean we can also close out apache/cordova-cli#306 as won't merge. No problems either way. |
|
@erisu I just wanted to be sure we're all on the same page regarding terminology. As I said before, since this is purely a convenience feature and I see no use in it either, I'm fine with removing this. And @dpogue seems to approve the removal as well. Last time I tried to use the CLI search of npm, I ended up reading an issue where they said that it's been kind of broken forever 😁 |
Platforms affected
none
What does this PR do?
cordova plugin searchcommandWhat testing has been done on this change?
npm test