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 the first argument as a module name #145

Merged
merged 2 commits into from
Jan 21, 2023
Merged

✨ Use the first argument as a module name #145

merged 2 commits into from
Jan 21, 2023

Conversation

Glutexo
Copy link
Owner

@Glutexo Glutexo commented Aug 23, 2022

The first CLI argument with a module name enables the writing of runnable modules. A future Parser module (#89) only needs a main method taking the root path as an argument.

The dynamic approach also allows testing of the CLI entry point.

The approach is naïve. It does check neither the parameter count nor whether a module exists. A missing argument or an incorrect module name, including a different case, causes an uncaught error.

With the new code in place, the escript has a new invocation syntax:

$ ./onigumo Downloader

The first CLI argument with a module name enables the writing of runnable modules. A future Parser module only needs a main method taking the root path as an argument.
@Glutexo Glutexo requested a review from nappex August 23, 2022 12:00
@Glutexo Glutexo self-assigned this Aug 23, 2022
@Glutexo Glutexo changed the title Use the first argument as a module name ✨ Use the first argument as a module name Aug 23, 2022
@Glutexo Glutexo added the enhancement New feature or request label Aug 23, 2022
@nappex
Copy link
Collaborator

nappex commented Jan 15, 2023

I've got an error:

$ ./onigumo Downloader
** (HTTPoison.Error) :nxdomain
    (httpoison 1.8.0) lib/httpoison.ex:156: HTTPoison.request!/5
    (onigumo 0.1.0) lib/onigumo_downloader.ex:24: Onigumo.Downloader.download_url/2
    (elixir 1.12.2) lib/stream.ex:572: anonymous fn/4 in Stream.map/2
    (elixir 1.12.2) lib/stream.ex:1559: Stream.do_element_resource/6
    (elixir 1.12.2) lib/stream.ex:1719: Enumerable.Stream.do_each/4
    (elixir 1.12.2) lib/stream.ex:649: Stream.run/1
    (elixir 1.12.2) lib/kernel/cli.ex:124: anonymous fn/3 in Kernel.CLI.exec_fun/2

I invoked these commands to prepare escript

$ mix escript
$ mix escript.build
$ mix compile

@nappex
Copy link
Collaborator

nappex commented Jan 15, 2023

Tried to find something about :nxdomain on official doc, unfortunately without success.

nxdomain error site:https://hexdocs.pm/httpoison -> googled

@nappex
Copy link
Collaborator

nappex commented Jan 15, 2023

From https://itigic.com/nxdomain-error-how-can-we-avoid-it/

What is the NXDOMAIN error

When we try to enter a web page and we get an error message indicating NXDOMAIN, it means that the domain name could not be resolved. This logically means that we cannot navigate through that site we are trying to access.

@nappex
Copy link
Collaborator

nappex commented Jan 15, 2023

I've tried to debug HTTPoison.get! in terminal (iex) according to tutorial on official doc.

Unfortunately I've got an error:

iex(1)> HTTPoison.start()
** (UndefinedFunctionError) function HTTPoison.start/0 is undefined (module HTTPoison is not available)
    HTTPoison.start()

I feel helplessly... Oh no my baby woke up seize

@nappex
Copy link
Collaborator

nappex commented Jan 15, 2023

If you want run
iex(1)> HTTPoison.start()

you have to run iex as:
$ iex -S mix

@nappex
Copy link
Collaborator

nappex commented Jan 15, 2023

nxdomain error is something what is expected because we use as input urls with domain onigumo.local, that domain does not exist in reality!

@nappex
Copy link
Collaborator

nappex commented Jan 15, 2023

We should consider to make special parser to parse arguments from cli to get module.

See https://elixirschool.com/en/lessons/intermediate/escripts
Then we can add more functionality to that parser, for example check numbers of args and so on.

@Glutexo
Copy link
Owner Author

Glutexo commented Jan 21, 2023

I agree that using OptionParser is the correct way to process the arguments. I chose to use plain pattern matching only to keep the changeset to the lowest level possible and switch to the smarter approach in the follow-up pull request.

Also the argument is rudimentary in its nature. It is a raw module name. Passing an invalid one raises an exception. That too is a sign of a temporary simplistic solution.

@Glutexo
Copy link
Owner Author

Glutexo commented Jan 21, 2023

Thanks for your review! 🙌🏻 Merging.

@Glutexo Glutexo merged commit 076353f into master Jan 21, 2023
@Glutexo Glutexo deleted the escript branch January 21, 2023 17:10
@Glutexo
Copy link
Owner Author

Glutexo commented Jan 23, 2023

Thanks for mentioning iex -S mix. ❤️ I was surprised that there was no mix console command, and there it is!

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

Successfully merging this pull request may close these issues.

2 participants