Skip to content

Conversation

@mzaffalon
Copy link
Contributor

`f()`. Use [`get!`](@ref) to also store the default value in the dictionary.
This is intended to be called using `do` block syntax
This is intended to be called using `do` block syntax, when the first argument
Copy link
Member

Choose a reason for hiding this comment

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

Not sure the extra thing about "longer than one-liner" needs to be added. The example below immediately contradicts it.

Copy link

@sosiristseng sosiristseng Jul 2, 2020

Choose a reason for hiding this comment

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

Not sure the extra thing about "longer than one-liner" needs to be added.

Maybe just put a reference to do syntax?

Edit: https://docs.julialang.org/en/v1/base/base/#do explained the syntax already.

Choose a reason for hiding this comment

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

Or "more-than-trivial throwaway functions". Not sure if that adds clarity or appropriate in the documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed with both comments.

"This is intended to be called using do block syntax that creates an anonymous function for f::Function (see do syntax)?"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ops, I just wanted to comment.

`f::Function` of `get!` is longer than one-liner, for readability.
```julia
get(dict, key) do
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps best to make this into a proper doctest?

@rfourquet
Copy link
Member

rfourquet commented Jul 3, 2020

If anything needs to be done here, I would favor removing the entire "This is intended to be called using do block syntax:" sentence. This is idiomatic Julia, I hope we will not add such a sentence to every function that supports a function as first parameter... Docstrings are not meant to replace the manual, they should be to the point, not bloated with information that belongs elsewhere.

@mzaffalon
Copy link
Contributor Author

@rfourquet I see your point: the manual is a trove of information and the do-syntax is explained there. On the other hand, adding more information to the docstring will certainly help people who do not read the full manual before using Julia and may point them to the correct section in the manual.

A better docstring will also prevent useless posts like mine in discourse that originated this PR.

@rfourquet
Copy link
Member

A better docstring will also prevent useless posts like mine in discourse that originated this PR.

I just had a rapid look to this discourse thread, but it seems that if the sentence related to do syntax was not there (as I suggest in the previous post), then there would not have been confusion. Sure it was an opportunity to learn about do syntax, but it's not specific to get!. Where to draw the line for what we explain in docstrings? A simple and efficient rule is to explain exactly what is specific to the documented function. I understand that for beginners it can be convenient to have redundant information, but including the definition of do blocks syntax in every docstrings of functions taking a function as first argument is a toll on every fluent Julia programmer who is fed with useless information.

@rfourquet rfourquet added the docs This change adds or pertains to documentation label Jul 3, 2020
@mzaffalon
Copy link
Contributor Author

mzaffalon commented Jul 3, 2020

Would it make sense to leave the example with time() to show how to have a default entry that may not be constant?
get!(time, dict, key)?

Return the value stored for the given key, or if no mapping for the key is present, store key => f(), and return f().

And in this context, is it clear from the documentation that f() is not executed twice? Would "Return the value stored for the given key, or if no mapping for the key is present, store key => val, where val = f() and return val." be better?

@rfourquet
Copy link
Member

Would it make sense to leave the example with time() to show how to have a default entry that may not be constant?
get!(time, dict, key)?

It looks fine to me to leave the current example. I would also keep it with the do syntax even if it's a contrived example, as it still suggests that do syntax is intended, and explicits that time() takes no argument, and hence that the first arg of get! is a function taking no argument. I would also put it under an # Examples heading.

And in this context, is it clear from the documentation that f() is not executed twice?

I guess it depends on the audience. It doesn't make sense to compute f() twice so this is unambiguous for me for example, but your alternate explicit formulation seems fine too.

@CameronBieganek
Copy link
Contributor

Hey, I just noticed this PR. I actually submitted a similar PR (#36964) last week, and it has already been merged. However, note that the focus of the new example in #36964 is not on clarifying the do block notation but on clarifying the intended purpose of the get! function.

@mzaffalon mzaffalon closed this Aug 19, 2020
@mzaffalon mzaffalon deleted the patch-2 branch August 19, 2020 04:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs This change adds or pertains to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants