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.

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

Jump to bottom

Improve Chain Documentation #4386

Merged
merged 12 commits into from
Feb 21, 2023
Merged

Improve Chain Documentation #4386

merged 12 commits into from
Feb 21, 2023

Conversation

reardonj
Copy link
Contributor

@reardonj reardonj commented Feb 1, 2023

This PR updates the Chain documentation to include links to API docs & other sections of the local site, as part of #2862

  • adds Laika configuration to build.sbt to use its API link feature
  • adds an mdoc variable to point to the API doc base URL. This is a workaround, because Laika can't link to NonEmptyChain, which is an alias for NonEmptyChainImpl.
  • Updates the Chain documentation to include links to other documentation pages & to API pages. References to types in the cats documentation are updated to point to those pages. References to undocumented types & scala base library types point to the API docs instead. The first reference to Chain & NonEmptyChain point to their API documentation.
  • Adds some headers to the Ior page to enable linking to its section about NonEmptyChain
  • Moves the How it Works and benchmarks sections to the bottom of the document. This is subjective, but in the current docs, there is a reference to NonEmptyChain just before it goes into all the benchmarks, but we don't actually find the Nec documentation until the end of the page. Moving the Nec section before these ones makes more sense to me as documentation for people just trying to use the type.
  • Misc. typo and spelling fixes.

@reardonj
Copy link
Contributor Author

reardonj commented Feb 1, 2023

I see git diff handles the section reorg poorly. I can back out that part if that eases review.

reardonj
reardonj commented Feb 1, 2023
View reviewed changes
docs/datatypes/chain.md
NonEmptyChain.fromChainAppend(Chain.empty[Int], 1)
NonEmptyChain.fromChainPrepend(1, Chain(2, 3))
```
[nec]: @API_LINK_BASE@/cats/data/index.html#NonEmptyChain:cats.data.NonEmptyChainImpl.type
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what the mdoc var is for. Laika can't link to this, but this seemed like a reasonable approximation of linking without hardcoding too much.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Laika's own ${vars} also don't work in links, I tried that first 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

Laika can't link to this

If you have a moment, might be good to open an issue for this in Laika.

johnynek
johnynek reviewed Feb 2, 2023
View reviewed changes
docs/datatypes/chain.md Outdated Show resolved Hide resolved
docs/datatypes/chain.md Outdated Show resolved Hide resolved
@reardonj
Tweaks based on feedback
b63d539 
- note that concat is also constant time
- add mention of groupByNec since it does exist, but it has no scaladoc
@reardonj
Update tables
432306a
@reardonj
final version of benchmarks
356c95d
johnynek
johnynek reviewed Feb 3, 2023
View reviewed changes
docs/datatypes/chain.md Outdated Show resolved Hide resolved
docs/datatypes/chain.md Outdated Show resolved Hide resolved
@reardonj
More tweaks
41d4a3f 
- add a little more meat to the intro paragraph
- add header for the motivation section
- call out Chain being a binary tree
- typos and formatting
johnynek
johnynek reviewed Feb 4, 2023
View reviewed changes
docs/datatypes/chain.md Outdated Show resolved Hide resolved
@armanbilge armanbilge changed the title Improve Chain Documentation Improve Chain Documentation Feb 4, 2023
@reardonj
Copy link
Contributor Author

reardonj commented Feb 5, 2023

I figured out you can make the API link say something other than "Chain", so we can put a marked API link. With latest commit it looks like this:
image

It's also possible to tuck it above the title:
image

It could also get put in a 'See Also' section at the bottom of the page, but that's much less discoverable.

armanbilge
armanbilge previously approved these changes Feb 11, 2023
View reviewed changes
Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

Overall LGTM, thanks for all your work on this, including the new benchmarks!

docs/datatypes/chain.md
NonEmptyChain.fromChainAppend(Chain.empty[Int], 1)
NonEmptyChain.fromChainPrepend(1, Chain(2, 3))
```
[nec]: @API_LINK_BASE@/cats/data/index.html#NonEmptyChain:cats.data.NonEmptyChainImpl.type
Copy link
Member

Choose a reason for hiding this comment

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

Laika can't link to this

If you have a moment, might be good to open an issue for this in Laika.

docs/datatypes/chain.md
Comment on lines 24 to 25
Well, `Vector` has its own problems and in this case it's unfortunately not that much faster than `List` at all. You can check [this blog post](https://www.lihaoyi.com/post/BenchmarkingScalaCollections.html#vectors-are-ok) by Li Haoyi for some deeper insight into `Vector`'s issues.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how relevant that blog post is for the 2.13 collections Vector. Maybe to 2.12, not sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will be interesting to see how relevant Chain remains after 2.13.11 Vector updates.

docs/datatypes/chain.md Show resolved Hide resolved
@Togoperez Togoperez mentioned this pull request Feb 15, 2023
Open
@armanbilge
Copy link
Member

armanbilge commented Feb 18, 2023 •
edited
Loading

Anyone have new thoughts on this? Otherwise I'll merge in a day or two, it's just docs after all.

Edit: where by "just" I mean it's not going to affect our compatibility guarantees 😜

@reardonj
Fix API documentation link
d1e8f62 
- this link is not working. When embedded in the markdown link, Chain loses capitalization, and gets redirected.
@reardonj
Copy link
Contributor Author

Anyone have new thoughts on this? Otherwise I'll merge in a day or two, it's just docs after all.

Edit: where by "just" I mean it's not going to affect our compatibility guarantees 😜

Was starting on the links for other pages today, and I can't get the link format I was using before to work :S

This is the best I can do without having to link manually:
image

armanbilge
armanbilge approved these changes Feb 21, 2023
View reviewed changes
Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

Thank you so much for your on-going work on Cats Docs 🥰

@armanbilge armanbilge merged commit 52c58d2 into typelevel:main Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@satorg satorg satorg left review comments

@johnynek johnynek johnynek left review comments

@armanbilge armanbilge armanbilge approved these changes

Assignees
No one assigned
Labels
documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@reardonj @armanbilge @johnynek @satorg