Skip to content

High level API docs - #105

Merged
simonmar merged 2 commits into
simonmar:masterfrom
nh2:high-level-api-docs
Feb 24, 2020
Merged

High level API docs#105
simonmar merged 2 commits into
simonmar:masterfrom
nh2:high-level-api-docs

Conversation

@nh2

@nh2 nh2 commented Feb 3, 2020

Copy link
Copy Markdown
Contributor

Based on #104.

This implements @simonmar's request from #104 (comment):

The documentation should start by introducing withAsync, and only later should talk about async and wait. It's a bigger restructuring, but I think starting the docs with some code that you shouldn't use is not the right thing at all

There are 2 commits in here on top of #104 (see their commit messages):

  1. Improve the prose in the introduction, add various smaller fixes.
  2. Re-order the haddock sections to make the point even clearer.

@nh2 nh2 changed the title High level api docs High level API docs Feb 3, 2020

@simonmar simonmar left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Looks good! Just a couple of minor comments

Comment thread Control/Concurrent/Async.hs Outdated
--
-- A slight improvement over the previous example is this:
-- * Waiting for 2 results ('concurrently').
-- * Waiting for /N/ many results ('mapConcurrently' / 'forConcurrently').

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

"Waiting for many results"?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment thread Control/Concurrent/Async.hs Outdated
-- > page2 <- wait a2
-- > ...
-- Click here to scroll to that section:
-- "Control.Concurrent.Async#high-level-utilities" (and help implement

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

is this pointer to Haddock relevant here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The problem is that haddock renders the link as just Control.Concurrent.Async, which is very confusing beacuse it suggests that it links to the full module, not a section, see screenshot:

image

I was a bit on the fence whether I should put this call to action to fix it in; I kept it in the first draft so you can see it and decide whether you think it's worth it or whether we should keep async's docs free from petitions to help with tooling :D

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think "Click here to scroll to that section" is enough of a hint (perhaps "jump" rather than "scroll" though), let's remove the link to the Haddock issue.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK, done.

@nh2
nh2 force-pushed the high-level-api-docs branch from 96ce3c1 to db41c89 Compare February 6, 2020 13:41
nh2 added 2 commits February 13, 2020 15:01
This implements Simon Marlow's feedback in PR simonmar#104:

> The documentation should start by introducing `withAsync`,
> and only later should talk about `async` and `wait`.
> It's a bigger restructuring, but I think starting the docs
> with some code that you shouldn't use is not the right thing at all

To make clear what functions people should usually use,
split the introduction into a "High-level" and "Low-level"
API, which is terminology I found very successful when it comes
to explaining when which function should be used.

Also add detail and improve markup in a few places.
This makes it easier for the user to understand which sections
they likely need _not_ read.
@nh2
nh2 force-pushed the high-level-api-docs branch from db41c89 to 1d96dc5 Compare February 13, 2020 14:01
@nh2

nh2 commented Feb 16, 2020

Copy link
Copy Markdown
Contributor Author

I've updated the PR to implement the feedback.

@simonmar
simonmar merged commit 1adc73c into simonmar:master Feb 24, 2020
@simonmar

Copy link
Copy Markdown
Owner

Thanks!

@vdukhovni vdukhovni left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Typographical nit

-- * If the operation throws an exception, then that exception is re-thrown
-- by 'wait'. This ensures property (1): No exception is swallowed.
-- * If an exception bubbles up through a 'withAsync', then the 'Async'
-- it spawned its 'cancel'ed. This ensures property (2): No thread is leaked.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

s/its cancelled/is cancelled/

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

PR #109

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants