Skip to content
New issue

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

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

Already on GitHub? # to your account

Make config raise specific domain specific error #176

Merged
merged 2 commits into from
Oct 27, 2022
Merged

Conversation

telphan
Copy link
Contributor

@telphan telphan commented Oct 27, 2022

The motivation for this PR is to be able to target these errors specifically.

@telphan telphan force-pushed the new_argument_error branch from 3abe19e to c77645f Compare October 27, 2022 12:32
@telphan
Copy link
Contributor Author

telphan commented Oct 27, 2022

We need to disable the requirement for:
Elixir 1.7 / OTP 23.0
Elixir 1.8 / OTP 23.0
Elixir 1.9 / OTP 23.0

As these are no longer supported

@telphan telphan merged commit 2202dee into main Oct 27, 2022
@telphan telphan deleted the new_argument_error branch October 27, 2022 13:33
@@ -20,6 +20,10 @@ defmodule Paginator.Config do
:total_count_limit
]

defmodule ArgumentError do
Copy link

Choose a reason for hiding this comment

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

fix: I don't think this is necessary since the Elixir stdlib for Exceptions already has this [1]? @ulissesalmeida: Is that correct?

[1] https://github.com/elixir-lang/elixir/blob/main/lib/elixir/lib/exception.ex#L807

Choose a reason for hiding this comment

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

Ahh yea, we should use def exception indeed 👍🏽

jesse-c pushed a commit that referenced this pull request Oct 27, 2022
This is to test my idea from
#176 (comment)
where I've said that I don't think this is necessary since the Elixir
stdlib for Exceptions already has this [1]?

[1] https://github.com/elixir-lang/elixir/blob/main/lib/elixir/lib/exception.ex#L807
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants