I'll take a look again today and land/comment.
On Thu, May 1, 2014 at 8:28 AM, Gorkem Ercan <[email protected]> wrote: > I would appreciate it if someone can have a final look and get this in.. I > have doc updates to follow. > Thanks > -- > Gorkem > > > On Fri, Apr 25, 2014 at 4:37 PM, Gorkem Ercan <[email protected] > >wrote: > > > FYI. Just updated the PR with separated save & restore commands. > > -- > > Gorkem > > > > > > On Thu, Apr 24, 2014 at 7:21 PM, Gorkem Ercan <[email protected] > >wrote: > > > >> I guess this is essentially moving save command as a flag to cordova > >> plugin * commands and restore becomes harder to discover. We need to > >> consider the platforms too since next stop is implementing "cordova > >> save/restore platforms" > >> -- > >> Gorkem > >> > >> > >> On Thu, Apr 24, 2014 at 5:05 PM, Michal Mocny <[email protected] > >wrote: > >> > >>> Gorkem, > >>> > >>> I also found an old JIRA duplicate issue for this which had listed an > old > >>> suggestion I think is interesting: > >>> https://issues.apache.org/jira/browse/CB-4624 > >>> > >>> Specifically, instead of introducing save/restore commands, we could > >>> mirror > >>> "npm install" and its use of "--save": > >>> - npm install -> cordova plugin add > >>> - restores deps > >>> - npm install foo -> cordova plugin add foo > >>> - install foo, but don't add it to deps > >>> - npm install foo --save -> cordova plugin add foo --save > >>> - install foo, and do add it to deps > >>> - npm install --save -> cordova plugin add --save > >>> - don't install anything, just save the current installed plugins > into > >>> deps > >>> > >>> Just something to consider. > >>> > >>> (note, just tried it again and npm install --save doesn't actually do > >>> what > >>> I want.. wonder if its supported..) > >>> > >>> -Michal > >>> > >>> > >>> On Wed, Apr 23, 2014 at 11:39 PM, Michal Mocny <[email protected]> > >>> wrote: > >>> > >>> > Gorkem, as an orthogonal issue to the syntax we use, I think there is > >>> > another small problem with this patch as-is. > >>> > > >>> > "cordova plugin add org.apache.cordova.file-transfer && cordova > plugin > >>> > save" would have my application explicitly depend on > >>> > org.apache.cordova.file. If in the future the dependency is > >>> > removed/moved/renamed, my app would explicitly try to install it when > >>> > running "cordova plugin restore". > >>> > > >>> > As a first version I think this is acceptable, but I think we may > want > >>> a > >>> > better solution eventually. > >>> > > >>> > > >>> > On Tue, Apr 22, 2014 at 1:18 PM, Michal Mocny <[email protected]> > >>> wrote: > >>> > > >>> >> > >>> >> > >>> >> > >>> >> On Tue, Apr 22, 2014 at 11:37 AM, Gorkem Ercan < > >>> [email protected]>wrote: > >>> >> > >>> >>> On Fri, Apr 18, 2014 at 7:21 PM, Michal Mocny <[email protected] > > > >>> >>> wrote: > >>> >>> > >>> >>> > On Fri, Apr 18, 2014 at 4:11 PM, Gorkem Ercan < > >>> [email protected] > >>> >>> > >wrote: > >>> >>> > > >>> >>> > > On Thu, Apr 17, 2014 at 3:46 PM, Michal Mocny < > >>> [email protected]> > >>> >>> > wrote: > >>> >>> > > > >>> >>> > > > Took a quick glance. General questions: > >>> >>> > > > - why the need for save? Why not just alter the list on each > >>> >>> cordova > >>> >>> > > > plugin add/rm? > >>> >>> > > > > >>> >>> > > > >>> >>> > > I do not want to force this workflow. Calling save does not > bring > >>> >>> much > >>> >>> > > overhead, considering plugin list does not probably change > >>> >>> constantly. > >>> >>> > > > >>> >>> > > >>> >>> > If you are going to make this choice, then I would not like to > >>> >>> > automatically install on prepare. There should be an explicit > >>> "load" > >>> >>> > command. (those names are wrong, but you get the point). > >>> >>> > > >>> >>> > Either we automatically manage plugin installs, or explicitly > >>> manage > >>> >>> them. > >>> >>> > I'm happy to start with explicit and support opt-in to automatic > >>> >>> handling > >>> >>> > later once we have confidence in the feature. > >>> >>> > > >>> >>> > >>> >>> Makes sense... adding a 'restore' command and removing from > prepare. > >>> We > >>> >>> can > >>> >>> add an auto-restore config in the next iteration. > >>> >>> > >>> >> > >>> >> Excellent, thanks. > >>> >> > >>> >> > >>> >>> > >>> >>> > >>> >>> > > >>> >>> > > >>> >>> > > > >>> >>> > > > >>> >>> > > > - wouldn't "cordova plugin rm foo && cordova prepare" > >>> re-install > >>> >>> that > >>> >>> > > > plugin right now? > >>> >>> > > > > >>> >>> > > > >>> >>> > > This workflow would require an explicit update to config.xml > if a > >>> >>> plugin > >>> >>> > is > >>> >>> > > persisted in there. This is a good point, I shall update > plugins > >>> rm > >>> >>> to > >>> >>> > > print a warning about it. > >>> >>> > > >>> >>> > > >>> >>> > > > >>> >>> > > > - why the name <feature> and not <dependency> ? I think this > >>> >>> > > functionality > >>> >>> > > > should overlap with the plugin.xml spec. > >>> >>> > > > > >>> >>> > > > > >>> >>> > > feature tag lives in the w3c widget spec which has advantages > for > >>> >>> those > >>> >>> > > (like myself) who does provide tools for editing config,xml. > >>> >>> > > > >>> >>> > > >>> >>> > We are no longer using that spec, and I as we move more > >>> functionality > >>> >>> from > >>> >>> > plugins.xml into config.xml we should strive to keep them in > line. > >>> It > >>> >>> also > >>> >>> > makes our docs easier. > >>> >>> > > >>> >>> > > >>> >>> Tools developers are people too :) I think plugin.xml and > config.xml > >>> has > >>> >>> two different audiences, I think there is a comparatively small > >>> >>> intersection of developers who are interested on both. At the > >>> moment, we > >>> >>> are pretty much within the w3c spec with the top-level config.xml > >>> and I > >>> >>> do > >>> >>> not see the benefit of breaking it. > >>> >>> > >>> >> > >>> >> Actually, I am thinking about tools devs. Namely, our tools devs > who > >>> >> should be using the same config parsing and handlers for dependency > >>> >> management, etc. > >>> >> > >>> >> Anyway. You are putting in the sweat and blood on this feature, so > >>> you > >>> >> get double the voting power on this as far as I'm concerned. > Still, I > >>> >> think we should bring this to the attention of the list to see how > >>> everyone > >>> >> feels. Sound fair? I'll start a quick thread. > >>> >> > >>> >> > >>> >>> > >>> >>> > >>> >>> > > >>> >>> > > > >>> >>> > > > >>> >>> > > > >>> >>> > > > I haven't put the PR through testing yet. > >>> >>> > > > > >>> >>> > > > > >>> >>> > > > On Thu, Apr 17, 2014 at 5:54 PM, Jesse < > >>> [email protected]> > >>> >>> > wrote: > >>> >>> > > > > >>> >>> > > > > Yeah, that looks weird. > >>> >>> > > > > > >>> >>> > > > > @purplecabbage > >>> >>> > > > > risingj.com > >>> >>> > > > > > >>> >>> > > > > > >>> >>> > > > > On Thu, Apr 17, 2014 at 2:46 PM, kamrik < > [email protected]> > >>> >>> wrote: > >>> >>> > > > > > >>> >>> > > > > > Github user kamrik commented on a diff in the pull > request: > >>> >>> > > > > > > >>> >>> > > > > > > >>> >>> > > > > >>> >>> > https://github.com/apache/cordova-cli/pull/165#discussion_r11755812 > >>> >>> > > > > > > >>> >>> > > > > > --- Diff: src/save.js --- > >>> >>> > > > > > @@ -0,0 +1,71 @@ > >>> >>> > > > > > +/** > >>> >>> > > > > > + Licensed to the Apache Software Foundation (ASF) > >>> >>> under one > >>> >>> > > > > > + or more contributor license agreements. See the > >>> >>> NOTICE > >>> >>> > file > >>> >>> > > > > > + distributed with this work for additional > >>> information > >>> >>> > > > > > + regarding copyright ownership. The ASF licenses > >>> this > >>> >>> file > >>> >>> > > > > > + to you under the Apache License, Version 2.0 > (the > >>> >>> > > > > > + "License"); you may not use this file except in > >>> >>> compliance > >>> >>> > > > > > + with the License. You may obtain a copy of the > >>> >>> License at > >>> >>> > > > > > + > >>> >>> > > > > > + http://www.apache.org/licenses/LICENSE-2.0 > >>> >>> > > > > > + > >>> >>> > > > > > + Unless required by applicable law or agreed to > in > >>> >>> writing, > >>> >>> > > > > > + software distributed under the License is > >>> distributed > >>> >>> on > >>> >>> > an > >>> >>> > > > > > + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS > >>> OF ANY > >>> >>> > > > > > + KIND, either express or implied. See the > License > >>> for > >>> >>> the > >>> >>> > > > > > + specific language governing permissions and > >>> >>> limitations > >>> >>> > > > > > + under the License. > >>> >>> > > > > > +*/ > >>> >>> > > > > > + > >>> >>> > > > > > +var cordova_util = require('./util'), > >>> >>> > > > > > + ConfigParser = require('./ConfigParser'), > >>> >>> > > > > > + path = require('path'), > >>> >>> > > > > > + xml =require('./xml-helpers') > >>> >>> > > > > > + Q = require('q'), > >>> >>> > > > > > + events = require('./events'); > >>> >>> > > > > > + > >>> >>> > > > > > +; > >>> >>> > > > > > + > >>> >>> > > > > > + > >>> >>> > > > > > +module.exports = function save(target){ > >>> >>> > > > > > + var projectHome = cordova_util.cdProjectRoot(); > >>> >>> > > > > > + var configPath = > >>> >>> cordova_util.projectConfig(projectHome); > >>> >>> > > > > > + var configXml = new ConfigParser(configPath); > >>> >>> > > > > > + var pluginsPath = path.join(projectHome, > 'plugins'); > >>> >>> > > > > > + var plugins = > cordova_util.findPlugins(pluginsPath); > >>> >>> > > > > > + > >>> >>> > > > > > + return Q.all(plugins.map(function(plugin){ > >>> >>> > > > > > + var currentPluginPath = > >>> path.join(pluginsPath,plugin); > >>> >>> > > > > > + var name = readPluginName(currentPluginPath); > >>> >>> > > > > > + var id = plugin; > >>> >>> > > > > > + var version = > >>> readPluginVersion(currentPluginPath); > >>> >>> > > > > > + var features = configXml.doc.findall('feature'); > >>> >>> > > > > > + for(var i=0; i<features.length; i++){ > >>> >>> > > > > > + if(features[i].attrib.name === name){ > >>> >>> > > > > > + events.emit('results', 'An entry for "'+ > >>> >>> plugin+ '" > >>> >>> > > > > already > >>> >>> > > > > > exists'); > >>> >>> > > > > > + return Q(); > >>> >>> > > > > > + } > >>> >>> > > > > > + } > >>> >>> > > > > > + configXml.addFeature(name, > >>> JSON.parse('[{"name":"id", > >>> >>> > > > > > "value":"'+id+'"},{"name":"version", > >>> >>> "value":"'+version+'"}]')); > >>> >>> > > > > > --- End diff -- > >>> >>> > > > > > > >>> >>> > > > > > I might be missing something, but why JSON.parse() > >>> rather > >>> >>> than > >>> >>> > > just > >>> >>> > > > > > literal array of objects? > >>> >>> > > > > > > >>> >>> > > > > > > >>> >>> > > > > > --- > >>> >>> > > > > > If your project is set up for it, you can reply to this > >>> email > >>> >>> and > >>> >>> > > have > >>> >>> > > > > your > >>> >>> > > > > > reply appear on GitHub as well. If your project does not > >>> have > >>> >>> this > >>> >>> > > > > feature > >>> >>> > > > > > enabled and wishes so, or if the feature is enabled but > not > >>> >>> > working, > >>> >>> > > > > please > >>> >>> > > > > > contact infrastructure at [email protected] or > >>> file a > >>> >>> JIRA > >>> >>> > > > > ticket > >>> >>> > > > > > with INFRA. > >>> >>> > > > > > --- > >>> >>> > > > > > > >>> >>> > > > > > >>> >>> > > > > >>> >>> > > > >>> >>> > > >>> >>> > >>> >> > >>> >> > >>> > > >>> > >> > >> > > >
