cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Grieve <agri...@chromium.org>
Subject Re: Review Request 15775: Add --src & --link to "cordova create"
Date Fri, 13 Dec 2013 02:38:49 GMT
Reviewboard accounts aren't connected to anything sadly. Just like JIRA,
and the Wiki, you need to create a new account.


On Thu, Dec 12, 2013 at 4:59 PM, Josh Soref <jsoref@blackberry.com> wrote:

> 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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message