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

More Dict and Set examples #33936

Closed
wants to merge 17 commits into from
Closed

More Dict and Set examples #33936

wants to merge 17 commits into from

Conversation

aminya
Copy link

@aminya aminya commented Nov 25, 2019

Ready to get merged...

@aminya aminya changed the title More Dict examples More Dict and Set examples Nov 25, 2019
@aminya aminya force-pushed the docImprove branch 3 times, most recently from 84478b3 to c24c74d Compare November 25, 2019 04:52
base/dict.jl Outdated Show resolved Hide resolved
base/dict.jl Outdated Show resolved Hide resolved
@jonas-schulze
Copy link
Contributor

The way you print the sets might be incompatible with #33300.

aminya added a commit to aminya/julia that referenced this pull request Nov 25, 2019
aminya added a commit to aminya/julia that referenced this pull request Nov 25, 2019
aminya added a commit to aminya/julia that referenced this pull request Nov 25, 2019
@aminya
Copy link
Author

aminya commented Nov 25, 2019

The way you print the sets might be incompatible with #33300.

I should check with nightly then.

Copy link
Contributor

@jonas-schulze jonas-schulze left a comment

Choose a reason for hiding this comment

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

I like the additional examples. 🙂

I just noticed that your examples for sets are no jldoctests. If that is intentional (as to not rely on a particular order of the elements when printing; there might even be another issue/PR for that) I think that's fine.

base/dict.jl Outdated Show resolved Hide resolved
base/dict.jl Outdated Show resolved Hide resolved
base/dict.jl Outdated Show resolved Hide resolved
aminya added a commit to aminya/julia that referenced this pull request Dec 4, 2019
@ViralBShah ViralBShah added the docs This change adds or pertains to documentation label Dec 5, 2019
aminya added a commit to aminya/julia that referenced this pull request Jan 28, 2020
aminya added a commit to aminya/julia that referenced this pull request Jan 28, 2020
aminya added a commit to aminya/julia that referenced this pull request Jan 28, 2020
@aminya
Copy link
Author

aminya commented Jan 28, 2020

This is ready to get merged.

base/set.jl Outdated Show resolved Hide resolved
base/dict.jl Outdated Show resolved Hide resolved
@KristofferC
Copy link
Member

This is ready to get merged.

Seems like all CI failed though.

base/dict.jl Outdated Show resolved Hide resolved
base/dict.jl Outdated Show resolved Hide resolved
base/dict.jl Outdated Show resolved Hide resolved
base/dict.jl Outdated Show resolved Hide resolved
base/dict.jl Outdated Show resolved Hide resolved
aminya added 13 commits February 6, 2020 05:18
Adding jldoctest setexamples

Set crossreference
Fixes JuliaLang#33936 (comment)
Fixes JuliaLang#33936 (comment)

using extended help for Dict

Fix get docstring

Fixes JuliaLang#33936 (comment)

dict.jl fixes

Fixes
JuliaLang#33936 (comment)

Fixes
JuliaLang#33936 (comment)

Changing Dict dynamic filling description
aminya added a commit to aminya/julia that referenced this pull request Feb 6, 2020
Using Julia 1.5 to get the examples output

Indentation fix

JuliaLang#33936 (comment)

Fixing jldoctests
aminya added a commit to aminya/julia that referenced this pull request Feb 6, 2020
Using Julia 1.5 to get the examples output

Indentation fix

JuliaLang#33936 (comment)

Fixing jldoctests
Using Julia 1.5 to get the examples output

Indentation fix

JuliaLang#33936 (comment)

Fixing jldoctests
base/abstractset.jl Outdated Show resolved Hide resolved
base/abstractset.jl Outdated Show resolved Hide resolved
base/abstractset.jl Outdated Show resolved Hide resolved
@@ -493,6 +561,12 @@ julia> get(d, "a", 3)
julia> get(d, "c", 3)
3
```

Using `get()` is the same as using `d[x]` syntax when the key exists in the collection. However, when the key doesn't exist, `get` returns the default value while `d[x]` throws an error.
Copy link
Member

Choose a reason for hiding this comment

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

This still just seems like a rewrite of the section above the example above this. I think it can be removed.

Copy link
Author

@aminya aminya Feb 20, 2020

Choose a reason for hiding this comment

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

The section above doesn't talk about the difference between d[x] and get()

base/set.jl Outdated Show resolved Hide resolved
doc/src/base/collections.md Show resolved Hide resolved
base/set.jl Outdated Show resolved Hide resolved
"red"
```

Since sets don't store repeated elements, `push!` will not have any effect for pushing the elements that are already in the set. Also because `Set` doesn't have a concept of "first" (unlike `Array`), using [`pushfirst!`](@ref) to insert an item at the beginning of a set will result in an error.
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this whole paragraph can be removed. This is mostly rehashing things that have already been mentioned.

Copy link
Author

@aminya aminya Feb 21, 2020

Choose a reason for hiding this comment

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

Here I am describing in the context of push!.


# Extended help

You can use [Generator](https://docs.julialang.org/en/v1/manual/arrays/#Generator-Expressions-1) and [Comprehension][https://docs.julialang.org/en/v1/manual/arrays/#Comprehensions-1] syntax to create dictionaries:
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a restatment of the first part of this docstring:

Given a single iterable argument, constructs a Dict whose key-value pairs
are taken from 2-tuples (key,value) generated by the argument.

Copy link
Author

Choose a reason for hiding this comment

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

No, it is not. Dict(x => x^2 for x = 0:1:4) is different from Dict([x => x^2 for x = 0:1:4])

Copy link
Author

@aminya aminya Feb 20, 2020

Choose a reason for hiding this comment

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

Also, now that I am thinking, the place of the 1st and 2nd examples should be changed. The 2nd example is 2X faster since it directly makes the Dict.

Examples

julia> Dict([("A", 1), ("B", 2)])
Dict{String,Int64} with 2 entries:
  "B" => 2
  "A" => 1

Alternatively, a sequence of pair arguments may be passed.

julia> d = Dict("A"=>1, "B"=>2)
Dict{String,Int64} with 2 entries:
  "B" => 2
  "A" => 1

Dict{Any,Any} with 0 entries
```

After creating an empty dictionary, fill it via a for loop:
Copy link
Member

Choose a reason for hiding this comment

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

This is the doctstring for the constructor Dict(). Now we are talking about modyfing an existing dict which feels out of scope for this docstring.

Copy link
Author

@aminya aminya Feb 20, 2020

Choose a reason for hiding this comment

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

This is in the Extended help section of Dict. Dict doesn't have a manual like Array does, so this docstring is the best place for talking about modifying a Dict or etc.

@aminya
Copy link
Author

aminya commented Feb 21, 2020

General note about this PR:
This is a documentation for those who are not familiar with Julia, and may not infer all the features and capabilities from a single sentence. So having more description and example doesn't harm the user, but instead gives more knowledge.

As an example see these

They all describe the ins and outs of the syntax plus having many different examples for different situations.

This PR is meant to fill the gap that exist for Julia for Dict and Set

@c42f
Copy link
Member

c42f commented Apr 1, 2020

Looking at the history of this PR, the discussion here has been very long for what I'd naively expect should be a simple change. Personally I glanced over these changes a few weeks ago and I had a feeling they "somehow didn't fit in", but couldn't express why.

However I was just thinking about the discussion at the end of #34226 and when I thought again of this issue it became clear what the problem is: @aminya is trying to write Tutorial documentation — in the sense of https://www.divio.com/blog/documentation (this is a must-read for anyone interested in good documentation) — but the Julia documentation currently has no place for tutorials.

Currently we have:

  • Reference documentation is contained in docstrings which are terse but complete. @aminya this is the reason you are seeing resistance for adding large amounts of examples to the docstrings. Docstrings are just not a good place for extended tutorial-like content.
  • Explanation is contained in the main Julia user manual (there is some small amount of tutorial content mixed in but that generally doesn't fit well with the style). As explanation, the manual currently doesn't serve beginners well, because tutorial content doesn't fit in nicely.
  • Tutorials and How-Tos do not have a natural home.

To resolve this in a good way I think we really need an explicit home for tutorial content in the manual which is separate from the docstrings and separate from the Explanation part of the manual. Combined with good cross linking between the sections I think we'll get the best outcome.

@aminya
Copy link
Author

aminya commented Apr 3, 2020

I tried to help to improve the documentation of Julia. But there is resistance for adding manual, extra information, examples, and things that are specifically useful for beginners (rehashing, redundant rewording, and all the other adjectives used in the comments). I had to argue with people even for one sentence. This like cold water on people's passion for contribution. It hurts to put time on something, go back and forth fixing stuff, and in the end getting the impression that "you know what? let's forget about it!"

Some of the changes are not even in the docstrings https://github.com/JuliaLang/julia/pull/33936/files#diff-e0b60ee9b69a6054f919078823363231. I think the vision towards documentation PRs should also change.

The last time that the doc project was touched was in 2017. This shows the status pretty much.
https://github.com/JuliaLang/julia/projects/8

Similar:
FluxML/Flux.jl#985
FluxML/Flux.jl#853

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Apr 3, 2020

The last time that the doc project was touched was in 2017. This shows the status pretty much.
https://github.com/JuliaLang/julia/projects/8

The only thing that shows is that we don't use the GitHub projects feature.

Doc improvements are very much appreciated and merged all the time without issue. In this case, a lot of feedback was given and you effectively rejected large amounts of it. If you won't accept feedback on a PR, then it may not get merged and the person who gave that feedback may get tired and go away.

I think the basic issue with this PR is that it adds a lot of content to doc strings, which are supposed to be fairly minimal reference-style explanations of how specific APIs work. A lot of the feedback which was rejected was pushing the changes in that direction. With a more robust support for short versus extended docstrings (see #34226), having more and longer examples might be fine, but as it is there's a tradeoff between being more "friendly" versus overwhelming people with too many examples and details.

If you were to separate this into the manual additions, which are less controversial, and some minimal doc string improvements, that PR could be merged quickly and the addition of lots of examples to doc strings could wait on the resolution of how to handle extended docs.

@ViralBShah
Copy link
Member

May I suggest closing this one and opening separate smaller PRs? I have been looking at many old doc PRs and merging them. In many cases I am doing the suggested work by the reviewers myself, which takes me more time than it would have taken the author.

It's hard to figure out the right answer. The quality of reviews for even small PRs is incredibly good, that leads to higher quality documentation that will literally help everyone who reads it. However, I can also see how it can be difficult for a new contributor.

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.

7 participants