Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check for incompatible CLI arguments #676

Closed
AlexandraRoatis opened this issue Oct 22, 2018 · 14 comments

Comments

@AlexandraRoatis
Copy link
Contributor

commented Oct 22, 2018

Introduction

This bounty request is geared towards improving the overall user experience of interacting with the Kernel through the command line options. We are constantly looking to improve and enhance features that will accelerate the development for Aion users.

Current situation

The current CLI implementation provides correct behavior by ignoring errors the user may make in selecting options that are incompatible with one another. When incompatible options are provided, only one of the options will be executed. The option that will be executed depends on the functionality ordering inside the class implementation, not on the order of the given arguments. This situation may lead some users to be puzzled by the execution behavior and can be remedied by printing warning messages in case of incompatible arguments as described below.

Task Description

  • Analyse the method call from the class Cli.java to determine which combinations of command line arguments are incompatible.
  • Add a new method checkArguments(Arguments givenOptions) to the class Cli.java that should be called after the command line parser call where the program arguments are checked and a warning message is printed signaling that incompatible options were given.
  • Example:
    • The current functionality allows only one of the account options to be executed by a program call. The option to be executed depends on the ordering inside the class, not on the order of the given arguments. Therefore if a user runs the command ./aion.sh -a list -a create the list accounts option will be ignored and the program will proceed to creating a new account. The new functionality introduced here should ensure that a meaningful message is printed for the user before the create account functionality is triggered. The message for this task would be:
      The given arguments «-a list» and «-a create» require incompatible tasks. Only «-a create» will be executed.
  • The functionality must be accompanied by unit tests for the checkArguments method that provide full coverage of the method and illustrate how it handles varying numbers of incompatible options.

Acceptance Criteria

  • The code implements the targets described above.
  • The code is well documented and fully covered by unit tests.
  • The code is compliant with Aion's code conventions.
  • The code gets merged into the main code repository.

Required Skill

Users must have the following skills and experience:

  • Java software development
  • Debugging and unit testing

Guidance

Details regarding this issue can be requested in comments.
Further technical support is available through Gitter.

@gitcoinbot

This comment has been minimized.

Copy link

commented Oct 22, 2018

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


This issue now has a funding of 580.0 AION (246.21 USD @ $0.42/AION) attached to it as part of the Aion Foundation fund.

@gitcoinbot

This comment has been minimized.

Copy link

commented Oct 22, 2018

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


Workers have applied to start work.

These users each claimed they can complete the work by 4 months, 2 weeks ago.
Please review their action plans below:

1) omarwanis has applied to start work (Funders only: approve worker | reject worker).

The Cli method checkArguments will keep a list in which incompatible commands are appended.
For each one of the incompatible commands (checked in order of execution), if the command is present in args, add it to the incompatible commands list mentioned above. After finishing the check, if the list has more than one element, print a message showing that the first command only will be executed and all the remaining ones will be discarded.
For extensibility, reflection could be used by adding the names of the methods in Arguments to be checked in order and they will be checked in that given order.

Learn more on the Gitcoin Issue Details page.

@Destiner

This comment has been minimized.

Copy link
Contributor

commented Oct 24, 2018

Hey, here's a quick report on my findings so far.

I looked at the Cli class, and here's what I found:

  1. Most of the arguments cause "do and exit (or error)" logic: they allow to do one thing, and then stop the execution.
  2. Given (1), most of the calls to CLI will only execute one action.

Based on this, I think the following will be the sanest solution:

  • Go through the given options to find the priority of the breaking task
  • Make list of skipped arguments by comparing their priority with the priority of breaking task
  • Show error message, listing all skipped arguments
  • Make sure there is a breaking task (otherwise no warning) and there is at least one skipped task (otherwise no warning).

Heading into implementation and testing.

@Destiner

This comment has been minimized.

Copy link
Contributor

commented Oct 24, 2018

Made the draft implementation. Few concerns that I have:

  • We hardcode task priority. If some task will be added or moved inside call method, we should not forget to update getBreakingTaskPriority and getSkippedTasks too. Probably we can't fix too much here.
  • Arguments gives us an abstraction of argument tokens, but to provide a list of skipped tasks, we need to hardcode them in getSkippedTasks. Would be nice if we can use something like Arguments.help.getOptionName(), but I didn't find the way to accomplish that.
  • We also don't track which variant user provided when we show the warning. Whether it's -i or --info, we'll show --info in warning.
@AlexandraRoatis

This comment has been minimized.

Copy link
Contributor Author

commented Oct 24, 2018

  1. I agree that there is not much that can be done about hardcoding the priorities. We will be careful when adding new ones.
  2. As far as I can tell the CLI parser doesn't allow this. An alternative would be to add the default names either as final variables under each option or as an object that can be accessed like help in the comment above.
  3. One could pass the initial args (given to the Cli class) to the checkArguments method to find the exact variant, but this is not important enough to turn into a requirement. Even the cli library we use to parse the arguments doesn't always do this.
@Destiner

This comment has been minimized.

Copy link
Contributor

commented Oct 27, 2018

Quick update. Looks like the implementation is mostly done, the code is going through review.

@Destiner

This comment has been minimized.

Copy link
Contributor

commented Nov 1, 2018

Seems like PR is merged @AlexandraRoatis.

@kzeine

This comment has been minimized.

Copy link
Contributor

commented Nov 1, 2018

Hey @Destiner, thanks for your great work on this bounty - it seems that gitcoin is not recognizing a submission, can you please make sure that you have submitted via gitcoin and I will make the payment ASAP. Feel free to reach me at karim@aion.network if you face any issues.

Thanks,

@Destiner

This comment has been minimized.

Copy link
Contributor

commented Nov 1, 2018

Hi @kzeine, oh no worries, it's totally ok as I didn't submit yet. Just wanted to make sure I finished the task. No hurry at all.

@kzeine

This comment has been minimized.

Copy link
Contributor

commented Nov 1, 2018

@Destiner thank you, yes the task has been finished & merged. We are ready to make the payment once you submit it!

Cheers,

@kzeine

This comment has been minimized.

Copy link
Contributor

commented Nov 2, 2018

Hi @Destiner, please confirm receipt of the bounty reward. Here is the tx ID: https://etherscan.io/tx/0xc078cf47daca16116b82cc18eee311baac1aa92ed158eb40c990c217d32c43fc

@Destiner

This comment has been minimized.

Copy link
Contributor

commented Nov 2, 2018

Hi @kzeine, yep, seems good. I guess I need to stop on this bounty now?

@kzeine

This comment has been minimized.

Copy link
Contributor

commented Nov 2, 2018

@Destiner Yes, that is correct. Thanks for your work - we look forward to more contributions!

@kzeine kzeine closed this Nov 2, 2018

@kzeine

This comment has been minimized.

Copy link
Contributor

commented Nov 2, 2018

The work for this bounty has been completed and merged for this issue.

@AlexandraRoatis AlexandraRoatis added this to the 0.3.2 milestone Nov 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.