Skip to content

Expect non_neg_integer instead of pos_integer #1571

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.

Sign up for GitHub

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

Closed
RemiBardon wants to merge 1 commit into from
Closed

Expect non_neg_integer instead of pos_integer #1571

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion exercises/concept/city-office/test/form_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ defmodule FormTest do
assert_spec(
{:check_length, 2},
["word :: String.t(), length :: non_neg_integer()", "String.t(), non_neg_integer()"],
":ok | {:error, pos_integer()}"
":ok | {:error, non_neg_integer()}"
Copy link
Copy Markdown
Contributor

@neenjaw neenjaw May 19, 2025

Choose a reason for hiding this comment

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

This task instructs a student to define a typespec for this function:

This is needed to check that the values of fields do not exceed the maximum allowed length.
It also tells you by how much the value exceeds the maximum.

A string can't exceed the maximum length by 0 and still be an error. So it was correct for the typespec to be a positive integer to exclude 0 from valid responses.

Copy link
Copy Markdown
Author

@RemiBardon RemiBardon May 19, 2025

Choose a reason for hiding this comment

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

If we look at implementation details, you’re correct, but in terms of types diff is a non_neg_integer(), per the spec of String.length/1. I was purely reasoning in types here.

Copy link
Copy Markdown
Author

@RemiBardon RemiBardon May 19, 2025

Choose a reason for hiding this comment

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

Coming from a Rust background, the tests didn’t behave as I was expecting, but maybe I’m wrong and it’s just not how it works in Elixir 🤷🏻‍♂️

Copy link
Copy Markdown
Author

@RemiBardon RemiBardon May 19, 2025

Choose a reason for hiding this comment

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

I’m not the experienced one here so I’ll be fine with whatever you find more correct.

Copy link
Copy Markdown
Contributor

@neenjaw neenjaw May 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think pos_integer() or non_neg_integer() is a type in the same sense as it is in rust. It's not a different data format, and pos_ and non_neg_ are communicating something about the expected value of the integer when it is part of the returned value. At this point there's no strict connection between the value returned and the type specified in the typespec attribute.

if we have non_neg_integer, we would have to cover the possibility of a {:error, 0} response when that's not an expected response from check_length/2

I think this will be more obvious in the future as set theoretic types are further fleshed out in future releases of the language, but for now this is is used as information for dialyzer, which performs static analysis on these types, function constructs, etc.

For now, I think it makes most sense to stick with pos_integer since 0 is excluded by the implementation.

Copy link
Copy Markdown
Author

@RemiBardon RemiBardon May 19, 2025

Choose a reason for hiding this comment

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

if we have non_neg_integer, we would have to cover the possibility of a {:error, 0} response when that's not an expected response from check_length/2

Good point. Makes sense why use pos_integer. Sorry for the disturbance and thank you for the links!

Copy link
Copy Markdown
Contributor

@neenjaw neenjaw May 19, 2025

Choose a reason for hiding this comment

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

No problem, no disturbance, thanks for the discussion!

)
end
end
Expand Down
Loading
⚡