cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Josh Soref <jso...@blackberry.com>
Subject Re: Review Request 15775: Add --src & --link to "cordova create"
Date Thu, 12 Dec 2013 21:59:38 GMT
Mark wrote:
>https://reviews.apache.org/r/15775/
>--link was a boolean before, now it accepts a path, so it's either
>--src=path or --link=path.
>Added a check in config_parser to make sure we are looking at an xml file
>that looks like Cordova config.xml to avoid overwriting some unrelated
>config.xml when symlinking to www dir.
>
>Same diff on github: https://github.com/kamrik/cordova-cli/compare/src_www

I can¹t figure out how to use review board (I guess the account isn¹t
connected to JIRA, is it connected to anything else?)

> [--src|source=<PATH>] ...... use custom www assets instead of the stock
>cordova hello-world.

Please use ³Cordova² not ³cordova² unless you mean ³./cordova² or some
other script.
(I¹m aware that there¹s a line in another file which has the same
convention, that¹s a bug, which is hopefully being fixed by some other
pending pull request, but if it¹s in flight, then it will miss yours)

> [--link=<PATH>] ............ as above, but symlink the custom www dir
>instead of copying.


Please don¹t use ³as above², it¹s asking for things to break when the next
person inserts a line into the help file.

+var path = require('path');
+var optimist; // required in try-catch below to print a nice error
message if it's not installed.


I believe general style would be:

var path = require('path'),
    optimist,
Š;

+    // If no inputArgs given, use preocess.argv but without the


Please don¹t introduce spelling errors, not even in comments.

> _ = require('lodash');


What is this, and why don¹t I see it in packages.json?


MacBook-Pro:docs $ node
> require('lodash')
Error: Cannot find module 'lodash'
    at Function.Module._resolveFilename (module.js:338:15)



+        if (inputArgs[0].match('node')) { // On Win first element is full
path to node.exe



This is way too hacky.

You¹re reordering a lot of unrelated code. I¹m very uncomfortable with
mixing rewrites and new features. I¹d personally prefer you split them
into distinct commits. ‹ It¹s making reviewing the changes here incredibly
painful.


+    if((r.tag !== 'widget') || !r.attrib || (r.attrib.xmlns !== xmlns)) {


There should probably be a space after `if`Š

-        if (contents.length == 0) {
+        if (contents.length === 0) {


If this is a style fix, please have it in a style-fix commit instead of
mixed with a refactor and a feature.

         } else if (contents.length == 1) {
Also, why didn¹t you change this??


-    shell.mkdir('-p', www_dir);


there¹s probably a good reason for this, but it¹s now so burried that
there¹s no way I can figure out what it is :(


---------------------------------------------------------------------
This transmission (including any attachments) may contain confidential information, privileged
material (including material protected by the solicitor-client or other applicable privileges),
or constitute non-public information. Any use of this information by anyone other than the
intended recipient is prohibited. If you have received this transmission in error, please
immediately reply to the sender and delete this information from your system. Use, dissemination,
distribution, or reproduction of this transmission by unintended recipients is not authorized
and may be unlawful.


Mime
View raw message