Skip to content

Rewrite InitializeReader and Analyze as truly synchronous methods - #290

Merged
MarkPflug merged 2 commits into
MarkPflug:mainfrom
0xced:true-sync
Mar 25, 2026
Merged

Rewrite InitializeReader and Analyze as truly synchronous methods#290
MarkPflug merged 2 commits into
MarkPflug:mainfrom
0xced:true-sync

Conversation

@0xced

@0xced 0xced commented Mar 25, 2026

Copy link
Copy Markdown
Contributor

This prevents the sync over async anti-pattern for the InitializeReader and Analyze methods.

Note that this source generator could be used on many more async methods in order to greatly reduce sync/async code duplication across the projects.

This prevents the [sync over async][1] anti-pattern for the `InitializeReader` and `Analyze` methods.

Note that this source generator could be used on many more async methods in order to greatly reduce sync/async code duplication across the projects.

[1]: https://github.com/davidfowl/AspNetCoreDiagnosticScenarios/blob/master/AsyncGuidance.md#warning-sync-over-async
@MarkPflug

Copy link
Copy Markdown
Owner

I think I'm more inclined to manually maintain a sync version of each method than take an external dependency on this source-gen package. I need to remind myself the scope of work that would be required to achieve that. I know sync over async can be problematic, but I thought it was mostly an issue in "hot path" code, and Initialize and Analyze should definitely not be high frequency which is why I haven't bothered with them up to this point.

@0xced 0xced changed the title Use the Sync Method Generator to automatically generate sync methods Rewrite InitializeReader and Analyze as truly synchronous methods Mar 25, 2026
@MarkPflug

Copy link
Copy Markdown
Owner

Apparently not much work. Thanks. I'll merge.

@MarkPflug
MarkPflug merged commit b187103 into MarkPflug:main Mar 25, 2026
1 check passed
@0xced
0xced deleted the true-sync branch March 25, 2026 20:53
@0xced

0xced commented Mar 29, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for merging this, too! Are you planning to release a new version of the Sylvan.Data.Csv package?

@MarkPflug

Copy link
Copy Markdown
Owner

Yes. I've been away on vacation the last few days. I'll get to it soon.

@MarkPflug

Copy link
Copy Markdown
Owner

New versions of Sylvan.Data and Sylvan.Data.Csv have been published with these changes. Thanks again for your contributions.

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.

2 participants