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

Use OptionParser to parse component #189

Merged
merged 5 commits into from
Oct 15, 2023
Merged

Conversation

nappex
Copy link
Collaborator

@nappex nappex commented Oct 3, 2023

fix #162
fix #188

I'be tried to be really consistent with existing code and make really the smallest diff as possible.
That's the one of reasons why I parse the component as positional arguments.

Nice thing was appeared, the OptionParser is not so good in parsing the positional arguments and its main goal is parsing switches (resp. options).

IMHO it is emphased by warning which you get from elixir, when you do not pass :switches or :strict, but these are required only for switches.

warning: not passing the :switches or :strict option to OptionParser is deprecate

@nappex nappex mentioned this pull request Oct 3, 2023
@Glutexo Glutexo self-requested a review October 15, 2023 16:31
@Glutexo Glutexo added the cli label Oct 15, 2023
Copy link
Owner

@Glutexo Glutexo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! 🎉 Great work! :shipit:

@@ -21,16 +21,20 @@ defmodule OnigumoCLITest do
Onigumo.CLI.main(["Downloader"])
end

test("run CLI with unknown argument") do
test("run CLI with invalid argument") do
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this is a separate change, but I see the change for consistency with the new “invalid switch” tests. It is okay to have it here.

@Glutexo Glutexo merged commit 2b2c2ef into Glutexo:master Oct 15, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use CLI component as positional argument Parse args from CLI with OptionParser.parse
2 participants