WIP: Improve unordered-containers API docs. #267

m-renaud · 2020-06-08T01:30:14Z

This integrates the content from #249 into the module docs themselves instead of having a separate Tutorial module.

Changes:

Improve top level module docs
Include examples for all functions
Call out complexity separately from function summary

m-renaud · 2020-06-08T01:30:42Z

@sjakobi wdyt about this compared to having separate external docs / tutorial modules?

sjakobi

I like this approach!

What's the motivation for shuffling around the complexity annotations though. Doesn't that make the documentation much longer?

Data/HashSet.hs

m-renaud · 2020-06-10T03:57:06Z

@sjakobi made some changes to formatting and added some comments as you suggested.

What's the motivation for shuffling around the complexity annotations though. Doesn't that make the documentation much longer?

The complexity of the function isn't the most important thing about the docs, and it feels kinda weird for the docs which describe what the functions do to start with O(something). This also leaves space for commentary on the complexity (see 'HashSet.size' docs) while keeping the formatting consistent. It does make the docs longer, but I don't think optimizing for doc length over organization is the right trade-off.

If this looks good to you I can make similar changes to the HashMap documentation.

m-renaud · 2020-06-10T04:01:50Z

Also, switch from bold to emphasis for the complexity clause on the functions at the bottom of the docs. I personally prefer the emphasis instead of bold because it doesn't compete with the function names for attention.

sjakobi · 2020-06-11T10:11:28Z

What's the motivation for shuffling around the complexity annotations though. Doesn't that make the documentation much longer?

The complexity of the function isn't the most important thing about the docs, and it feels kinda weird for the docs which describe what the functions do to start with O(something). This also leaves space for commentary on the complexity (see 'HashSet.size' docs) while keeping the formatting consistent. It does make the docs longer, but I don't think optimizing for doc length over organization is the right trade-off.

I haven't really made up my mind about it, but I'd prefer to discuss it separately, i.e. not as a part of this PR.

Also, switch from bold to emphasis for the complexity clause on the functions at the bottom of the docs. I personally prefer the emphasis instead of bold because it doesn't compete with the function names for attention.

Which bold complexity clauses does this refer to?

m-renaud · 2020-06-11T22:34:23Z

I haven't really made up my mind about it, but I'd prefer to discuss it separately, i.e. not as a part of this PR.

Sure, I can move them back to being inline.

Which bold complexity clauses does this refer to?

Complexity: O(n)

vs.

Complexity: O(n)

sjakobi

I like this!

Data/HashSet.hs

Data/HashSet/Base.hs

Data/HashSet.hs

sjakobi

Lovely! Just a few more nits.

I'd be interested in what @emilypi and @treeowl think then.

Data/HashSet.hs

sjakobi · 2020-06-19T17:49:41Z

Data/HashSet.hs

+recommended way to define the instance is using generics, for which you'll need
+the @DeriveGeneric@ GHC extension.
+
+Note: You'll need to enable the @DeviceGeneric@ language extension, either via
+@:set -XDeriveGeneric@ in GHCi or @\{\-\# DeriveGeneric \#\-\}@ in your Haskell source
+code.


I think it's slightly unfortunate to go into so much detail here when the canonical documentation on this is in hashable: http://hackage.haskell.org/package/hashable-1.3.0.0/docs/Data-Hashable.html#g:3

Maybe just add a link?!

No need to add anything. If @m-renaud wants that in the ReadTheDocs stuff, that's his prerogative, but it doesn't belong in the Haddocks.

@treeowl I think the current idea is not to add a ReadTheDocs page, but instead to include some examples and a bit of introductory documentation in the main haddocks.

I think there's some value in including an example of using u-c with custom datatypes, but I mostly feel that it shouldn't duplicate documentation that is or should be in hashable instead…

Yeah, my rationale for including a bit more than a link out was to prevent documentation hops, especially given that u-c is likely one of the first packages that someone new to the language may encounter. It does add a bit more content to the docs but having a full working example in one place without having to piece things together can be very useful.

Additionally, the canonical documentation doesn't mention what to do if you're in ghci, so I thought including it here would be helpful as well for those not familiar with extensions. I don't think we should assume that anyone reading these docs knows that :set -XExtensionName is how to enable extensions in ghci; yes they're bound to figure it out eventually the more time you spend in the Haskell ecosystem, but one extra sentence to avoid a stackoverflow or reddit post seems like a good trade-off to me.

@sjakobi @treeowl please either 👍 or 👎 this comment:

👍 - rationale sounds good, lets leave this

👎 - still don't agree, please remove details

I'm very happy with the section in its current state.

u-c is likely one of the first packages that someone new to the language may encounter

This is surprising to me! Why do you think new users will encounter it so soon? u-c always seemed fairly niche compared to containers to me.

treeowl · 2020-06-20T16:19:08Z

I'm a bit torn. I agree with much of your rationale, but I don't we want the u-c Haddocks to become an "introduction to Haskell". Would it make sense to put a full example with very little explanation in the module intro? Maybe. I'm sure we don't want to get into "how to enable extensions in GHCi". That's just far out of scope here.

m-renaud · 2020-06-20T16:30:56Z

@treeowl how's this?

m-renaud · 2020-06-20T16:36:08Z

@sjakobi should I do the HashMap docs in this PR as well or a follow-up?

sjakobi · 2020-06-20T16:39:43Z

@sjakobi should I do the HashMap docs in this PR as well or a follow-up?

Let's make a decision on HashSet first.

sjakobi

A few more last tweaks.

I think the header docs are really nice now. Good examples, but not too long.

Data/HashSet.hs

m-renaud · 2020-06-22T15:17:16Z

Done. ptal!

sjakobi · 2020-06-22T15:25:40Z

Nearly done, IMHO! Great job!

m-renaud · 2020-06-26T00:18:45Z

Sorry for the delay, updated typeclass references. If there's nothing else I can squash and merge.

m-renaud · 2020-06-26T00:21:34Z

jk, linknig to "Data.Eq.Eq" isn't valid :/

sjakobi · 2020-06-26T00:24:25Z

jk, linknig to "Data.Eq.Eq" isn't valid :/

It should be 'Data.Eq.Eq' I think, with single quotes or alternatively backticks. I'll take another look tomorrow.

sjakobi · 2020-06-26T06:51:57Z

jk, linknig to "Data.Eq.Eq" isn't valid :/

It should be 'Data.Eq.Eq' I think, with single quotes or alternatively backticks. I'll take another look tomorrow.

Note that haddock might still produce a warning since the identifier is not in scope. But it should be able to produce the correct link anyway.

Also, please include Hashable: 'Data.Hashable.Hashable'.

sjakobi · 2020-07-04T01:14:38Z

@treeowl Could you please review?

@m-renaud If you need help with the final touches to the markup, I can prepare a PR against yours.

sjakobi · 2020-07-04T01:15:18Z

@treeowl Could you please review?

~~Any~~ And @emilypi, too!? :)

emilypi · 2020-07-04T01:55:52Z

I'll have time this weekend @sjakobi 😄

m-renaud · 2020-07-04T02:41:49Z

I'll have time this weekend. Eq and Ord linked properly but Hashable didn't for some reason :/

m-renaud · 2020-07-05T16:59:46Z

@sjakobi at least locally I can't get Hashable to link, any suggestions?

sjakobi · 2020-07-05T18:02:12Z

@sjakobi at least locally I can't get Hashable to link, any suggestions?

It actually works for me when I build the documentation with cabal haddock --enable-documentation.

What command did you use? I suspect you maybe just didn't build the haddocks for hashable too.

sjakobi

LGTM! :)

Thanks for all the hard work and especially your readiness to find a compromise! 👍

m-renaud · 2020-07-06T02:57:43Z

For sure, thanks for the thorough review! I'll squash the commits and wait for the others to review before merging :)

- More information in introduction (basic operations, and using HashSet with custom data types) - Examples alongside function docs

sjakobi · 2020-07-17T13:08:24Z

@treeowl, @emilypi I'd hate to see this PR languish. Could you please review?

If there are no reviews, I'll merge this on Monday (2020-07-20) or soon after.

sjakobi · 2020-07-20T10:34:24Z

Thanks again, @m-renaud! :)

m-renaud · 2020-07-20T14:14:52Z

No problem! Should I do the same for HashMap next?

sjakobi · 2020-07-20T14:17:45Z

Should I do the same for HashMap next?

IMHO, yes please! :)

I'd feel more comfortable if @treeowl and @emilypi would signal their approval too though.

m-renaud added the documentation label Jun 8, 2020

m-renaud requested a review from sjakobi June 8, 2020 01:30

sjakobi reviewed Jun 9, 2020

View reviewed changes

Data/HashSet.hs Outdated Show resolved Hide resolved

Data/HashSet.hs Outdated Show resolved Hide resolved

Data/HashSet.hs Outdated Show resolved Hide resolved

Data/HashSet.hs Outdated Show resolved Hide resolved

sjakobi reviewed Jun 15, 2020

View reviewed changes

sjakobi reviewed Jun 19, 2020

View reviewed changes

sjakobi reviewed Jun 20, 2020

View reviewed changes

Data/HashSet.hs Outdated Show resolved Hide resolved

Data/HashSet.hs Outdated Show resolved Hide resolved

Data/HashSet.hs Outdated Show resolved Hide resolved

sjakobi approved these changes Jul 5, 2020

View reviewed changes

Improve HashSet API docs.

08175c1

- More information in introduction (basic operations, and using HashSet with custom data types) - Examples alongside function docs

m-renaud force-pushed the m-renaud-haddocks branch from 4d55200 to 08175c1 Compare July 6, 2020 04:10

sjakobi mentioned this pull request Jul 19, 2020

Rename internal modules and expose them #283

Merged

sjakobi merged commit afcbc77 into haskell-unordered-containers:master Jul 20, 2020

sjakobi mentioned this pull request Jul 20, 2020

Add external, beginner friendly docs #173

Open

WIP: Improve unordered-containers API docs. #267

WIP: Improve unordered-containers API docs. #267

Conversation

m-renaud commented Jun 8, 2020

m-renaud commented Jun 8, 2020

sjakobi left a comment

Choose a reason for hiding this comment

m-renaud commented Jun 10, 2020 • edited Loading

m-renaud commented Jun 10, 2020

sjakobi commented Jun 11, 2020

m-renaud commented Jun 11, 2020

sjakobi left a comment

Choose a reason for hiding this comment

sjakobi left a comment

Choose a reason for hiding this comment

sjakobi Jun 19, 2020

Choose a reason for hiding this comment

treeowl Jun 19, 2020

Choose a reason for hiding this comment

sjakobi Jun 19, 2020

Choose a reason for hiding this comment

m-renaud Jun 20, 2020

Choose a reason for hiding this comment

sjakobi Jun 20, 2020

Choose a reason for hiding this comment

treeowl commented Jun 20, 2020

m-renaud commented Jun 20, 2020

m-renaud commented Jun 20, 2020

sjakobi commented Jun 20, 2020

sjakobi left a comment

Choose a reason for hiding this comment

m-renaud commented Jun 22, 2020

sjakobi commented Jun 22, 2020

m-renaud commented Jun 26, 2020

m-renaud commented Jun 26, 2020

sjakobi commented Jun 26, 2020

sjakobi commented Jun 26, 2020 • edited Loading

sjakobi commented Jul 4, 2020

sjakobi commented Jul 4, 2020 • edited Loading

emilypi commented Jul 4, 2020

m-renaud commented Jul 4, 2020

m-renaud commented Jul 5, 2020

sjakobi commented Jul 5, 2020

sjakobi left a comment

Choose a reason for hiding this comment

m-renaud commented Jul 6, 2020

sjakobi commented Jul 17, 2020

sjakobi commented Jul 20, 2020

m-renaud commented Jul 20, 2020

sjakobi commented Jul 20, 2020

m-renaud commented Jun 10, 2020 •

edited

Loading

sjakobi commented Jun 26, 2020 •

edited

Loading

sjakobi commented Jul 4, 2020 •

edited

Loading