stratos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Imesh Gunaratne <im...@apache.org>
Subject Re: DISCUSS: usage of flags in stratos cli 4.1
Date Sat, 14 Mar 2015 14:16:03 GMT
Hi Martin,

Thanks for the feedback. Yes "flagOptions" would be ok, may be we can add a
description in the method comment to describe its purpose.

Thanks

On Thu, Mar 12, 2015 at 6:58 AM, Martin Eppel (meppel) <meppel@cisco.com>
wrote:

>  Hi Imesh,
>
>
>
> I don’t disagree, I do agree – no problem with changing the name of the
> variable but “options” simply doesn’t work (it won’t compile), we need to
> find another, non-clashing name.
>
>
>
> What about “flagOptions” ?
>
>
>
> Regards
>
>
>
> Martin
>
> *From:* Imesh Gunaratne [mailto:imesh@apache.org]
> *Sent:* Wednesday, March 11, 2015 6:13 PM
>
> *To:* dev
> *Subject:* Re: DISCUSS: usage of flags in stratos cli 4.1
>
>
>
> Hi Martin,
>
>
>
> Thanks for the response. Still "cmdOptions" does not sound (at least to
> me) as they are passed by the shell when running the CLI in none
> interactive mode.
>
>
>
> For a new developer it would be difficult to understand why this
> mergeOptions() method is executed and why we are reading command options
> twice. If we put proper variable names it would be much easier to
> understand. WDYT?
>
>
>
> Thanks
>
>
>
> On Wed, Mar 11, 2015 at 2:19 AM, Martin Eppel (meppel) <meppel@cisco.com>
> wrote:
>
> In general sounds good but I think naming it “options” won’t work as the
> cmd classes  already have a field named “options” and they will clash, what
> about “cmdOptions” ?
>
>
>
> Thanks
>
>
>
> Martin
>
>
>
> e.g.:
>
>
>
> …
>
> public class AddAutoscalingPolicyCommand implements
> Command<StratosCommandContext> {
>
>
>
>     private static final Logger logger =
> LoggerFactory.getLogger(AddAutoscalingPolicyCommand.class);
>
>
>
>     private final Options options;
>
>
>
> …
>
> *From:* Imesh Gunaratne [mailto:imesh@apache.org]
> *Sent:* Tuesday, March 10, 2015 1:08 PM
> *To:* dev
> *Subject:* Re: DISCUSS: usage of flags in stratos cli 4.1
>
>
>
> Hi Martin,
>
>
>
> I reviewed your fix and it looks good. The change you have done to include
> the options passed by the command line when running the CLI straightaway
> from the shell is correct.
>
>
>
> I would like to propose one change: you have named the new argument as
> "already_parsed_opts":
>
>
>
> *int *execute(T context, String[] args, Option[] already_parsed_opts) *throws
> *CommandException;
>
>
>
> May be we need to use standard Java naming convention here. Moreover
> "alreadyParsedOpts" does not give me a proper meaning, shall we rename it
> to "options"? Then anyone who would implement a command would know that the
> execute method itself passes options and we can also use
> commandLine.getOptions() to read options passed when running in
> interactive mode.
>
>
>
> Thanks
>
>
>
> On Tue, Mar 10, 2015 at 10:52 AM, Martin Eppel (meppel) <meppel@cisco.com>
> wrote:
>
> I pushed the bug fix in commit 06deaadc63a9756e7701f5173ba00847aec24c4a –
> please review.
>
> Although it seems that currently we are not using any commands with the
> flag only option it might still prove to be useful in case it will be
> needed.
>
>
>
> If you see an issue or concern please feel free to revert,
>
>
>
> Thanks
>
>
>
> Martin
>
>
>
> *From:* Martin Eppel (meppel)
> *Sent:* Monday, March 09, 2015 12:01 PM
> *To:* dev@stratos.apache.org
> *Subject:* DISCUSS: usage of flags in stratos cli 4.1
>
>
>
> Hi,
>
>
>
> In 4.1 do we still support cli commands which accept flags only, similar
> to “unsubscribe-cartridge –f” in 4.0 ? I went through all the cli commands
> but don’t see any “flag” as command option (unless I missed it) .
>
>
>
> The reason I am asking is that in 4.0.0  there was an issue with the
> command parser which missed to properly parse flags when there were invoked
> in the command mode (although it worked in the  interactive cli mode). I
> have a fix for this issue (ported from 4.0.0 private branch), however, if
> we don’t support flags then there is no need to push it upstream unless we
> plan to do so in the future?
>
>
>
> Thanks
>
>
>
> Martin
>
>
>
>
>
> --
>
> Imesh Gunaratne
>
>
>
> Technical Lead, WSO2
>
> Committer & PMC Member, Apache Stratos
>
>
>
>
>
> --
>
> Imesh Gunaratne
>
>
>
> Technical Lead, WSO2
>
> Committer & PMC Member, Apache Stratos
>



-- 
Imesh Gunaratne

Technical Lead, WSO2
Committer & PMC Member, Apache Stratos

Mime
View raw message