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

Use CSafeLoader when possible #1150

Merged
merged 6 commits into from
Jan 16, 2024
Merged

Use CSafeLoader when possible #1150

merged 6 commits into from
Jan 16, 2024

Conversation

wouterzwerink
Copy link
Contributor

Motivation

yaml.CSafeLoader is significantly faster. However, it is not always available. The proposed change is to use it whenever it is available with a fall back to yaml.SafeLoader (current loader).

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

No additional tests required, current tests should cover yaml being parsed correctly.

Fixes

What issue does this PR fix? Use https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue to link this PR to a corresponding issue.

Fixes #1149

Related PRs

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@odelalleau odelalleau self-requested a review January 16, 2024 14:22
Copy link
Collaborator

@odelalleau odelalleau left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good but can you please add a news fragment about this change?

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@wouterzwerink
Copy link
Contributor Author

Thanks, this looks good but can you please add a news fragment about this change?

Done, let me know if that looks good to you! By the way, it might be worth adding something about news fragments in CONTRIBUTING.md

Copy link
Collaborator

@odelalleau odelalleau left a comment

Choose a reason for hiding this comment

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

Should be good with this minor change

news/1150.feature Outdated Show resolved Hide resolved
@odelalleau
Copy link
Collaborator

Thanks, this looks good but can you please add a news fragment about this change?

Done, let me know if that looks good to you! By the way, it might be worth adding something about news fragments in CONTRIBUTING.md

Good point, see #1151 (unfortunately I can't add you as reviewer)

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: Olivier Delalleau <[email protected]>
Copy link
Collaborator

@odelalleau odelalleau 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!

@odelalleau odelalleau merged commit b5badbd into omry:master Jan 16, 2024
6 checks passed
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.

Use yaml.CSafeLoader whenever possible for speedups
2 participants