cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From david-barth-canonical <...@git.apache.org>
Subject [GitHub] cordova-lib pull request: Cb 9590 - Ubuntu support for the new plu...
Date Thu, 03 Dec 2015 15:59:44 GMT
Github user david-barth-canonical commented on a diff in the pull request:

    https://github.com/apache/cordova-lib/pull/349#discussion_r46569876
  
    --- Diff: cordova-lib/src/plugman/platforms/ubuntu.js ---
    @@ -29,6 +29,54 @@ function toCamelCase(str) {
         }).join('');
     }
     
    +function getPluginXml(plugin_dir) {
    +    var et = require('elementtree'),
    +    fs = require('fs'),
    +    path = require('path');
    +
    +    var pluginxml;
    +    var config_path = path.join(plugin_dir, 'plugin.xml');
    +
    +    if (fs.existsSync(config_path)) {
    +        // Get the current plugin.xml file
    +        pluginxml = et.parse(fs.readFileSync(config_path, 'utf-8'));
    +    }
    + 
    +    return pluginxml;
    +}
    +
    +function findClassName(pluginxml, plugin_id) {
    +    var class_name;
    +
    +    // first check if we have a class-name parameter in the plugin config
    +    if (pluginxml) {
    +	var platform = pluginxml.find("./platform/[@name='ubuntu']/");
    +	if (platform) {
    +	    var param = platform.find("./config-file/[@target='config.xml']/feature/param/[@name='ubuntu-package']");
    +	    if (param && param.attrib) {
    +		class_name = param.attrib.value;
    +		return class_name;
    +	    }
    +	}
    +    }
    +
    +    // fallback to guess work, based on the plugin package name
    +
    +    if (plugin_id.match(/\.[^.]+$/)) {
    +        // old-style plugin name
    +        class_name = plugin_id.match(/\.[^.]+$/)[0].substr(1);
    +        class_name = toCamelCase(class_name);
    +    } else {
    +	match = plugin_id.match(/cordova\-plugin\-([\w\-]+)$/);
    +        if (match && match.length > 0)
    +	    class_name = match[0].substr(15);
    +	else
    +            class_name = toCamelCase(class_name);
    +    }
    +
    +    return class_name;
    +}
    --- End diff --
    
    There is a new PR at https://github.com/apache/cordova-lib/pull/349 with
    your suggested fixes. Thanks for the review.
    
    On Thu, Dec 3, 2015 at 12:45 AM, Tim Barham <notifications@github.com>
    wrote:
    
    > In cordova-lib/src/plugman/platforms/ubuntu.js
    > <https://github.com/apache/cordova-lib/pull/349#discussion_r46494465>:
    >
    > > +    // fallback to guess work, based on the plugin package name
    > > +
    > > +    if (plugin_id.match(/\.[^.]+$/)) {
    > > +        // old-style plugin name
    > > +        class_name = plugin_id.match(/\.[^.]+$/)[0].substr(1);
    > > +        class_name = toCamelCase(class_name);
    > > +    } else {
    > > +	match = plugin_id.match(/cordova\-plugin\-([\w\-]+)$/);
    > > +        if (match && match.length > 0)
    > > +	    class_name = match[0].substr(15);
    > > +	else
    > > +            class_name = toCamelCase(class_name);
    > > +    }
    > > +
    > > +    return class_name;
    > > +}
    >
    > Thanks @david-barth-canonical <https://github.com/david-barth-canonical>!
    > Some comments:
    >
    >    - Generally the Cordova codebase uses curly braces even around single
    >    line blocks.
    >    - There are some tabs in there that should be converted to spaces.
    >    - The match variable needs a var.
    >    - Should be calling toCamelCase() for the scenario where we match
    >    cordova-plugin-...
    >    - Where we don't match cordova-plugin-..., class_name is not defined
    >    (should be using plugin_id).
    >    - Since we will always end up calling toCamelCase(), the logic for the
    >    second half of this method can be simplified to:
    >
    >     // fallback to guess work, based on the plugin package name
    >     if (plugin_id.match(/\.[^.]+$/)) {
    >         // old-style plugin name
    >         class_name = plugin_id.match(/\.[^.]+$/)[0].substr(1);
    >     } else {
    >         var match = plugin_id.match(/cordova\-plugin\-([\w\-]+)$/);
    >         if (match && match.length > 0) {
    >             class_name = match[0].substr(15);
    >         } else {
    >             class_name = plugin_id;
    >         }
    >     }
    >
    >     return toCamelCase(class_name);
    >
    > Also, since the original PR has been merged, can you recreate this as a
    > new PR which will just include the changes in your last commit (and changes
    > in response to feedback, of course).
    >
    > —
    > Reply to this email directly or view it on GitHub
    > <https://github.com/apache/cordova-lib/pull/349/files#r46494465>.
    >



---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org


Mime
View raw message