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

Improved argument parsing #17

Closed
wants to merge 1 commit into from
Closed

Improved argument parsing #17

wants to merge 1 commit into from

Conversation

hrshbh
Copy link

@hrshbh hrshbh commented Oct 19, 2019

Fixes #16

Fixed the argument parsing to allow parameters to be entered in any order.
In Progress:
Test cases need to be created.

@javier-lopez
Copy link
Member

Hello @Hrishabh23 ,

Thank you for working on this, unfortunately it seems the change isn't enough to make the command to run successfully:

❯ ./static-get -i top -p /tmp/opt 
Option '-p' requires a parameter -pno available packages at s.minos.io/archive, it may be an incorrect url or could be down temporarily

@hrshbh
Copy link
Author

hrshbh commented Oct 21, 2019

Hi @javier-lopez,
The problem was due to another previously present typo/bug, which prevented static-get of any package starting with t.
Coincidentally, you chose top, which helped spot the typo/bug. Programs with any other first letter would have been okay.

@javier-lopez
Copy link
Member

javier-lopez commented Oct 22, 2019

Hello @Hrishabh23

I just reviewed your changes and noticed the original project had been misreporting the test results for a while, so I fixed the test reporting until it went green and pushed your changes over it, it seems your code is breaking the suite test in some ways, could you review and fix the issues?, then it will require to add its additional tests to ensure the new improved argument parsing is working as expected, I know it can be kind of cumbersome, so I'll completely understand if you prefer to abort the PR.

@javier-lopez
Copy link
Member

Basically the issue is that when executing:

| >>static-get --download .
| >>head -1
| >>test X = Xstatic-get: missing arguments

The test is expecting static-get: missing arguments which I think is logical since no package has been requested, however with your changes the tool doesn't abort the execution.

javier-lopez added a commit that referenced this pull request Oct 22, 2019
@javier-lopez
Copy link
Member

I also cherry picked your fix for the #17 (comment) issue, 🍻 , so you will require to merge/rebase against https://github.com/minos-org/minos-static or just create a new PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

allow parameters to be specified in any order
2 participants