cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From TimBarham <...@git.apache.org>
Subject [GitHub] cordova-lib pull request: Cb 9590 - Ubuntu support for the new plu...
Date Wed, 02 Dec 2015 23:45:27 GMT
Github user TimBarham commented on a diff in the pull request:

    https://github.com/apache/cordova-lib/pull/349#discussion_r46494465
  
    --- 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 --
    
    Thanks @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:
    
    ```js
        // 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).


---
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