Skip to content

Conversation

@backuitist
Copy link
Contributor

I've integrated some of your comments ;)

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Look i've changed the link. But you might want to remove it as it points at http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/ForkJoinPool.html ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think there is a license problem with pointing back to Oracle's Java documentation.

Please break these two sentences into two lines.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm having second thoughts about mentioning FJ pool here.
The current ExecutionContext.global implementation, even though it's unlikely to change,
should be mentioned at most in the ScalaDoc API.
Otherwise, our documentation is too flaky, and it becomes hard to maintain the crossreferences.

This doc should discuss only the high-level usage and meaning of execution contexts,
not the specific implementation.
It is also unclear if we should mention minThreads, numThreads and maxThreads configuration settings,
since they are not mentioned explicitly in the API docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest that you refactor this section, so that all mention of ForkJoinPool is put in a separate section,
entitled e.g. "Execution Contexts based on Fork/Join Pools".

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I disagree, references of ExecutionContext.global are all over that documentation. So you might be right in theory, but in practice people will be using (and abusing) the global EC. Documenting how this works and flagging potential issues isn't low-level details.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I would suggest that you refactor this section, so that all mention of ForkJoinPool is put in a separate section,
entitled e.g. "Execution Contexts based on Fork/Join Pools".

That's actually what I did by wrapping FJP details into the The Global Execution Context section.

@SethTisue
Copy link
Member

review by @viktorklang and @axel22?

Copy link
Contributor

Choose a reason for hiding this comment

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

We normally write the type ascription immediately after the identifier, without the space:

val inverseFuture: Future[Matrix] = ... 

Choose a reason for hiding this comment

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

I recommend making the example take the EC as an implicit parameter to foster a culture of getting the EC passed in rather than defining it locally.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree - the more idiomatic version below could be sufficient.

@axel22
Copy link
Contributor

Thanks a lot for taking the effort to improve the documentation to this extent!
Please address my comments before we can merge this in.

Choose a reason for hiding this comment

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

I'd suggest:

is a placeholder object for a value that may not yet exist.

@SethTisue
Copy link
Member

@backuitist interested in getting this across the finish line...?

@backuitist
Copy link
ContributorAuthor

Sorry, that went off my radar.

@backuitistbackuitistforce-pushed the execution-context branch 2 times, most recently from a34a8a3 to ec26babCompareAugust 24, 2015 09:07
@SethTisue
Copy link
Member

@axel22@viktorklang lgty now?

@axel22
Copy link
Contributor

I would fix that one last colon, but it looks good to me.

@SethTisue
Copy link
Member

yay, across the finish line! thank you @backuitist for your great patience, thank you reviewers. (@viktorklang, it's of course not too late for further changes.)

SethTisue added a commit that referenced this pull request Sep 11, 2015
@SethTisueSethTisue merged commit fcb6c7b into scala:masterSep 11, 2015
@viktorklang
Copy link

@SethTisue :-)

Sign up for freeto 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.

5 participants

@backuitist@SethTisue@axel22@viktorklang@phaller