Skip to content

Special forms doctests #14435

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

dmitrykleymenov
Copy link
Contributor

Rewrote some code snippets to remove side effects and compiler warnings. Also added a couple of tests and made them more consistent in try/1

rescue
UndefinedFunctionError -> nil
end
Rescue a single exception without binding the exception to a variable
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Rescue a single exception without binding the exception to a variable
Rescue a single exception without binding the exception to a variable:

rescue
x in [UndefinedFunctionError] -> nil
end
Rescue any of the given exception without binding
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Rescue any of the given exception without binding
Rescue any of the given exception without binding:

...> end
:rescued

Rescue and bind the exception to the variable "x"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Rescue and bind the exception to the variable "x"
Rescue and bind the exception to the variable "x":

Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency with your other comment, do you mean:

Suggested change
Rescue and bind the exception to the variable "x"
Rescue and bind the exception to the variable `x`:

...> end
[:rescued, true]

Rescue several errors differently
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Rescue several errors differently
Rescue different errors with separate clauses:

:rescued_arithmetic_error

Rescue all kinds of exceptions and bind the rescued exception
to the variable "x"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
to the variable "x"
to the variable `x`:

iex> try do
...> :returned
...> after
...> send(self(), :send_from_after_block)
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure I like this new version, it requires the user to understand message passing in order to grasp it, while IO.puts is more "natural".

Copy link
Contributor Author

@dmitrykleymenov dmitrykleymenov Apr 14, 2025

Choose a reason for hiding this comment

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

It prints the message to stdout along with dots from passing tests(for now there are only dots). Is it ok to have messages there or should it be reverted to plain code? My point was to minimize side effects, but since after block never returns a value, it's hard to show the block call without side effects

Copy link
Member

Choose a reason for hiding this comment

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

Do you have an opinion, @sabiwara?

Copy link
Contributor Author

@dmitrykleymenov dmitrykleymenov Apr 14, 2025

Choose a reason for hiding this comment

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

And one more thing: I'm not sure but think try/1 is more or less an advanced feature, so highly likely a reader knows about message passing in general. Even without this knowledge, send, self, and receive are maximum self-descriptive. Just thoughts

Copy link
Contributor

Choose a reason for hiding this comment

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

@josevalim I agree that it looks more natural and much simpler.
Since by definition after is about side effects, and IO.puts is the simplest example, I think it makes sense not to have a doctest at all cost here?

Every other idea I come up with is more advanced and/or weird/non-idiomatic: process dict, agent, ETS ...
The only one that doesn't feel too bad and illustrates a typical use of after could be: create a tmp file and clean it up in after? But that's not something we typically do in doctests perhaps.

And one more thing: I'm not sure but think try/1 is more or less an advanced feature, so highly likely a reader knows about message passing in general. Even without this knowledge, send, self, and receive are maximum self-descriptive. Just thoughts

I don't think that's necessarily true though - especially for people with experience in other languages, who would typically be aware of try/finally / defer constructs but might still not know much about message passing.

Copy link
Contributor Author

@dmitrykleymenov dmitrykleymenov Apr 15, 2025

Choose a reason for hiding this comment

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

I don't think that's necessarily true though - especially for people with experience in other languages, who would typically be aware of try/finally / defer constructs but might still not know much about message passing.

Thank you for your feedback, you are right. So, revert to plain code or tmp file

Copy link
Member

Choose a reason for hiding this comment

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

Plain code :)

Copy link
Member

@josevalim josevalim left a comment

Choose a reason for hiding this comment

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

One comment about message passing and we are good to go!

...> end
:rescued

Rescue and bind the exception to the variable "x"
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency with your other comment, do you mean:

Suggested change
Rescue and bind the exception to the variable "x"
Rescue and bind the exception to the variable `x`:

iex> try do
...> :returned
...> after
...> send(self(), :send_from_after_block)
Copy link
Contributor

Choose a reason for hiding this comment

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

@josevalim I agree that it looks more natural and much simpler.
Since by definition after is about side effects, and IO.puts is the simplest example, I think it makes sense not to have a doctest at all cost here?

Every other idea I come up with is more advanced and/or weird/non-idiomatic: process dict, agent, ETS ...
The only one that doesn't feel too bad and illustrates a typical use of after could be: create a tmp file and clean it up in after? But that's not something we typically do in doctests perhaps.

And one more thing: I'm not sure but think try/1 is more or less an advanced feature, so highly likely a reader knows about message passing in general. Even without this knowledge, send, self, and receive are maximum self-descriptive. Just thoughts

I don't think that's necessarily true though - especially for people with experience in other languages, who would typically be aware of try/finally / defer constructs but might still not know much about message passing.

...> IO.puts(:stderr, "Unexpected message received")
...> after
...> 10 ->
...> "return this after 10 milliseconds"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the original message was nicer to explain?

Suggested change
...> "return this after 10 milliseconds"
...> "No message in 10 milliseconds"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants