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

Move input filename to config #45

Merged
merged 6 commits into from
Feb 7, 2022
Merged

Move input filename to config #45

merged 6 commits into from
Feb 7, 2022

Conversation

Glutexo
Copy link
Owner

@Glutexo Glutexo commented Jan 22, 2022

An input filename is a configuration value, it does not have to be a module constant. This extraction allows to use this value in tests too.

With this change, I removed the @testfile_with_urls attribute from the tests. Its purpose was to expose an otherwise inaccessible value in the Onigumo module. With @input_file moved to the config, this is no longer necessary, as it’s accessible with Application.get_env(:onigumo, :input_file).

load_urls/1 takes the filename as an argument and did not have to use the global value even before this change.

Fixes #31.

An input filename is a configuration value, it does not have to be a
module constant. This extraction allows to use this value in tests
too.
@Glutexo Glutexo requested a review from nappex January 22, 2022 10:34
@Glutexo Glutexo self-assigned this Jan 22, 2022
@Glutexo
Copy link
Owner Author

Glutexo commented Jan 29, 2022

Merged current elixir into this. Resolved conflicts.

I introduced a new change from the previous version. Instead of using a hardcoded "urls.txt" value in the tests, I load the value from the Application configuration. Even though in the tests it does not matter what the path actually is, the Application environment provide a sane value and prevents us from having hardcoded ones.

@Glutexo
Copy link
Owner Author

Glutexo commented Feb 3, 2022

Merged current elixir into this. Resolved conflicts, extended the usage of the config variable to the new tests.

@Glutexo
Copy link
Owner Author

Glutexo commented Feb 5, 2022

Merged current elixir into this. Resolved conflicts, extended the usage of the config variable to the new tests again.

Nice new piping has emerged.

Application.get_env(:onigumo, :input_path)
|> load_urls()
|> download(http, path)

test/onigumo_test.exs Show resolved Hide resolved
@Glutexo Glutexo merged commit 6a8a541 into elixir Feb 7, 2022
@Glutexo Glutexo deleted the config branch February 7, 2022 11:26
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.

Extract input file name to Config
2 participants