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

Changed BackgroundWorkers to async await #728

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

tombogle
Copy link
Contributor

@tombogle tombogle commented Oct 2, 2020

This change is Reviewable

@tombogle tombogle self-assigned this Oct 2, 2020
@tombogle tombogle changed the title Changed BackgroundWorkers in Project to async await Changed BackgroundWorkers to async await Oct 2, 2020
Copy link
Contributor

@andrew-polk andrew-polk left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @AndrewChristensenFCBH and @tombogle)


GlyssenEngine/Project.cs, line 1466 at r1 (raw file):

ParseAndIncludeBooks

Should this be named ParseAndIncludeBooksAsync?


GlyssenEngine/Project.cs, line 1528 at r1 (raw file):

ProjectState = ProjectState.UsxComplete | (ProjectState & ProjectState.WritingSystemRecoveryInProcess);

Should this line get moved into GuessAtQuoteSystem?
It doesn't matter now because this is the only caller. But someone looking just at the (albeit private) API might think GuessAtQuoteSystemAsync and GuessAtQuoteSystem are the same except for synchronicity.


GlyssenEngine/Project.cs, line 1549 at r1 (raw file):

m_projectMetadata.ParserVersion = kParserVersion;
			ProjectState = ProjectState.Parsing;

Same comment as for GuessAtQuoteSystemAsync above.

@tombogle
Copy link
Contributor Author

tombogle commented Oct 6, 2020


GlyssenEngine/Project.cs, line 1466 at r1 (raw file):

Previously, andrew-polk wrote…
ParseAndIncludeBooks

Should this be named ParseAndIncludeBooksAsync?

Could be. I only added the Async suffix for the methods where I needed a synchronous and asynchronous version. Not even sure this branch is a "keeper"....

@tombogle
Copy link
Contributor Author

tombogle commented Oct 6, 2020


GlyssenEngine/Project.cs, line 1528 at r1 (raw file):

Previously, andrew-polk wrote…
ProjectState = ProjectState.UsxComplete | (ProjectState & ProjectState.WritingSystemRecoveryInProcess);

Should this line get moved into GuessAtQuoteSystem?
It doesn't matter now because this is the only caller. But someone looking just at the (albeit private) API might think GuessAtQuoteSystemAsync and GuessAtQuoteSystem are the same except for synchronicity.

Yes it should. There is another caller.

Added error-handler to Paratext-based constructor so Vessel can specify its own error handling.
Copy link
Contributor

@andrew-polk andrew-polk left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @andrew-polk and @AndrewChristensenFCBH)

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

Successfully merging this pull request may close these issues.

3 participants