On Thu, Apr 17, 2014 at 3:46 PM, Michal Mocny <mmo...@chromium.org> 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. > - 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. > I haven't put the PR through testing yet. > > > On Thu, Apr 17, 2014 at 5:54 PM, Jesse <purplecabb...@gmail.com> wrote: > > > Yeah, that looks weird. > > > > @purplecabbage > > risingj.com > > > > > > On Thu, Apr 17, 2014 at 2:46 PM, kamrik <g...@git.apache.org> 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 infrastruct...@apache.org or file a JIRA > > ticket > > > with INFRA. > > > --- > > > > > >
