You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@cordova.apache.org by "Parashuram N (MS OPEN TECH)" <pa...@microsoft.com> on 2015/01/06 04:22:24 UTC

Cordova --list option implementation

Forking from Re: [DISCUSS] Tools Release to continue discussion.

+1 to Andrew suggestion that adding a top level command just for listing
targets would be an overkill. Also, people usually want to see the
supported targets when they run, so it makes sense to expose it closer to
where it gets used.

If we decide that cordova target add could be used to create new targets,
that could be a separate top level command, can `cordova run ‹list` could
be the alias for `cordova target platform list` command (though I am not a
fan of aliasing commands).

Josh, is your concern that ‹list throws an error in case of blackberry? We
could fix that by either

1a- Ignoring unknown flags like other platforms - and document the
ignoring part as the default behavior
1b - or change other platforms to throw error on unsupported flags. In
this case, ‹list would do the check to see if ‹list is implemented first.

2. Implement ‹list for blackberry, thereby not having error on any
platform. We could then have a separate discussion about what happens when
a flag is not supported by a platform.

It would be great if we can let users use the ‹list functionality with
minimal disruption to any platform.


On 1/5/15, 6:43 PM, "Andrew Grieve" <ag...@chromium.org> wrote:

>Josh - thanks for pointing out that the change isn't working as intended.
>I
>did some testing before merging, but didn't try the error conditions.
>
>I've just reverted the one to CLI that enables the flag. Let's move
>forward
>with releasing :)
>
>Also noticed that if you have an older-than-master version of android
>installed in your project, then you get:
>
>$ cordova run --list
>Running command:
>/Users/agrieve/git/cordova/cordova-cli/foo/platforms/android/cordova/run
>--list
>ERROR : Run option '--list' not recognized.
>ERROR running one or more of the platforms: Error:
>/Users/agrieve/git/cordova/cordova-cli/foo/platforms/android/cordova/run:
>Command failed with exit code 2
>You may not have the required environment or OS to run this project
>
>I actually do like the syntax of "cordova run --list", since the run
>command is the thing that --target is relevant to anyways. Maybe
>"--list-targets" would be more explicit? Adding  "cordova target add"
>would
>be overstepping I think. You can add Android emulator targets as well, but
>us writing a wrapper for the logic would just make things more complicated
>I think.
>
>I do think that having cordova-lib look for the existence of
>the list-emulator-images and list-devices scripts makes sense, and just
>have it call them directly.
>
>
>On Mon, Jan 5, 2015 at 5:29 PM, Josh Soref <js...@blackberry.com> wrote:
>
>> Murat Sutunc wrote:
>> >1) When provided with an unknown extra parameter, such as --list in
>>this
>> >case, all the platforms ignore it. This looks like the expected
>>behavior
>> >as there are several issues in Jira related to it. (ex. See bug
>>CB-6676 -
>> >
>> 
>>https://issues.apache.org/jira/browse/CB-6676?jql=project%20%3D%20CB%20AN
>>D
>> >%20text%20~%20%22ignore%20parameter%22). Exit code 1 on Blackberry
>>seems
>> >like a bug.
>>
>> https://wiki.apache.org/cordova/CommandLineToolingDesign#Errors
>>
>>
>> * 1: not implemented / unsupported command
>>
>>
>> I believe that what windows phone was doing was correct. And if someone
>>is
>> changing the contract, they failed to update the contract.
>>
>> >
>> >2) Introducing 'target' as a top level option seems like a new
>>proposal.
>> >My thoughts were to avoid having a new top level command for listing
>> >devices but I would like to hear others opinions on this as well. I
>>also
>> >think 'target' might not be the best choice of keyword here as it's
>> >already part of 'run' and it's easy to get confused:
>> >       cordova run --target=FOO
>> >       cordova target --list
>>
>> Not --list, just plain "list". As in "cordova platform list", and
>>"cordova
>> plugin list", it's a commonly used idiom in cordova.
>>
>> >3) I don¹t understand the 'cordova target add' command completely. Is
>>it
>> >an alias to 'cordova platform add'?
>>
>> No.
>>
>> For blackberry10 you can configure a "target" which is a name for a set
>>of
>> settings (including an ip address/dns name, whether it is a
>> device/simulator, and a device password). You can then use run
>> --target=foo.
>>
>> >4) 'Each platform should already support the list-* commands' is
>> >currently not true. firefoxos, browser, Ubuntu don¹t support it.
>>
>> Those platforms aren't complete, it's part of the contract, but we
>>already
>> have a good way of handling missing components of lib/ ‹ if they don't
>> exist (we can check for this), or aren't implemented (they return a
>> standard value).
>>
>> The way you've implemented it with run, there's no way to look before
>>you
>> leap, which results in the failure that you introduced for blackberry10.
>>


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

Re: Cordova --list option implementation

Posted by Andrew Grieve <ag...@chromium.org>.

I think 1.b sounds perfect.

On Thu, Jan 8, 2015 at 5:48 PM, Murat Sutunc <mu...@microsoft.com> wrote:

> I think we have a couple of options here:
>
> Option 1 - Adding --list as an optional parameter to cordova run
> List is related to run and there’s not that much need to introduce another
> top level command for this . Considering all the previous discussions we
> had I can see two potential implementations for this:
>                 A) look at the parameters of run call in platform level
> and take action
>                 B) look at the parameters of run call in lib level and
> take action
> Option 1.A results in errors or unexpected behavior on platforms that do
> not support --list (including older versions).
> Option 1.B involves parsing arguments in
> cordova-lib\cordova-lib\src\cordova\run.js (along with the cli). From this
> point instead of calling project_root/platforms/<platform>/cordova/run we
> can call project_root/platforms/<platform>/cordova/lib/list-* and so on.
>
> Option 2 - Adding ‘list’ as an optional parameter to a new command.
> List can be put under a new top level command such as target. This design
> involves adding a new top level command and corresponding handler in
> cordova-lib. Similar to run, the code with logic will reside somewhere like
> cordova-lib\cordova-lib\src\cordova\target.js.
>
> I would like to get some feedback about these options.
>
> -----Original Message-----
> From: Murat Sutunc [mailto:muratsu@microsoft.com]
> Sent: Tuesday, January 6, 2015 5:42 PM
> To: dev@cordova.apache.org
> Subject: RE: Cordova --list option implementation
>
> The feedback is definitely very valuable. I was trying to understand the
> issues with the existing design and once the impact is realized all the
> commits are immediately reversed.
> I'm currently looking into your suggestions and will try to come up with a
> better design.
>
> -----Original Message-----
> From: Josh Soref [mailto:jsoref@blackberry.com]
> Sent: Tuesday, January 6, 2015 10:40 AM
> To: dev@cordova.apache.org
> Subject: Re: Cordova --list option implementation
>
> Parashuram wrote:
> >Josh, is your concern that ‹list throws an error in case of blackberry?
>
> No. my concern is that the way this was done was wrong <PERIOD>.
> It doesn't work for *ANY* project that exists today with *ANY* platform.
>
> Cordova's design is to be backwards compatible. The code that was written
> absolutely failed that.
>
> I tried to hint at that in my review comments.
>
> But that was totally ignored.
>
> Note that list threw an error in android (@released) too (as Andrew
> belatedly noted).
>
> >We could fix that by either
> >1a- Ignoring unknown flags like other platforms - and document the
> >ignoring part as the default behavior
>
> This is too late. You can't really change the behavior because
> cordova-cli/cordova-lib needs to support old projects with old platforms.
>
> >1b - or change other platforms to throw error on unsupported flags. In
> >this case, ‹list would do the check to see if ‹list is implemented first.
>
> You can't do this for the same reason that we can't do 1a.
>
> >2. Implement ‹list for blackberry, thereby not having error on any
> >platform. We could then have a separate discussion about what happens
> >when a flag is not supported by a platform.
>
> It's not specific to blackberry10, it applies to all existing platforms
> which happen to have the underlying support but which shipped without an
> imaginary change to run.js
>
> >It would be great if we can let users use the ‹list functionality with
> >minimal disruption to any platform.
>
> Correct. And this is trivially done -- put the code into cordova-lib and
> have it run the scripts from the platforms, instead of requiring changes to
> platforms — they already did the work they need to do (they are free to
> replace their existing impls w/ node.js impls if they haven't already, but
> that's transparent to cordova-lib).
>  B KKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKKCB
>   [  X  ܚX K  K[XZ[
>    ] ][  X  ܚX P  ܙ ݘK \ X  K ܙ B  ܈ Y  ] [ۘ[    [X[     K[XZ[
>    ] Z [    ܙ ݘK \ X  K ܙ B
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
> For additional commands, e-mail: dev-help@cordova.apache.org
>