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

Rewrite extensions in Rust #721

Merged
merged 19 commits into from
Aug 9, 2023
Merged

Rewrite extensions in Rust #721

merged 19 commits into from
Aug 9, 2023

Conversation

sdispater
Copy link
Owner

@sdispater sdispater commented Jul 18, 2023

This PR is the first introduction to Rust in the codebase.

It is a complete rewrite of existing C extensions and paves the way to more speed improvements by rewriting other parts of the codebase in the future.

Note that if there is still a pure Python version of these extensions and will always be for the foreseeable future.

Be aware that this PR introduces small backwards incompatibilities when parsing ISO8601 strings:

  • Basic time representation now requires a leading T, for instance T201205
  • Fractional hours and minutes are no longer supported. It was technically supported before the behavior was incorrect so support has been removed for now but will be reintroduced in the future.

Pull Request Check List

  • Added tests for changed code.
  • Updated documentation for changed code.

@codspeed-hq
Copy link

codspeed-hq bot commented Jul 18, 2023

CodSpeed Performance Report

Merging #721 will degrade performances by 18.61%

Comparing refactor/rust (d29ce1b) with master (06fb2e9)

Summary

❌ 1 regressions
✅ 1 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark master refactor/rust Change
test_format 2.3 ms 2.8 ms -18.61%

@sdispater sdispater force-pushed the refactor/rust branch 3 times, most recently from 929c2bb to 11d3b30 Compare July 18, 2023 20:26
@sdispater sdispater force-pushed the refactor/rust branch 8 times, most recently from 30835dc to a44aa76 Compare August 4, 2023 22:56
@sdispater sdispater marked this pull request as ready for review August 4, 2023 23:17
@sdispater sdispater requested a review from Secrus August 4, 2023 23:18
Copy link
Collaborator

@Secrus Secrus left a comment

Choose a reason for hiding this comment

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

Some issues here and there, but overall, nice job. I am no Rust expert, so I just looked through it trusting that the test suite checks if it works correctly.

.github/workflows/tests.yml Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
build.py Outdated Show resolved Hide resolved
pendulum/helpers.py Show resolved Hide resolved
pendulum/parser.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
rust/helpers.rs Show resolved Hide resolved
Copy link

@Maneren Maneren left a comment

Choose a reason for hiding this comment

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

Great initiative, seeing C rewritten to Rust always makes me happy. I suggested some minor refactorings (partially suggested by clippy::pedantic) but otherwise very good job. There are still few noticeable remnants of C that don't really fit into Rust (eg. the null char) but they require more complex changes and it isn't really necessary to do that.

Also, I am quite new to reviewing (got here fairly randomly) so sorry if there's anything wrong in that regard.

rust/parsing.rs Outdated Show resolved Hide resolved
rust/parsing.rs Outdated Show resolved Hide resolved
rust/parsing.rs Outdated Show resolved Hide resolved
rust/parsing.rs Outdated Show resolved Hide resolved
rust/parsing.rs Outdated Show resolved Hide resolved
rust/python/types/timezone.rs Outdated Show resolved Hide resolved
rust/parsing.rs Outdated Show resolved Hide resolved
rust/parsing.rs Outdated Show resolved Hide resolved
rust/parsing.rs Outdated Show resolved Hide resolved
rust/parsing.rs Outdated Show resolved Hide resolved
@sdispater
Copy link
Owner Author

Thanks for the thorough review @Maneren! I applied all of your suggestions.

@Maneren
Copy link

Maneren commented Aug 6, 2023

Happy to help

@sdispater sdispater requested a review from Secrus August 6, 2023 20:52
Copy link
Collaborator

@Secrus Secrus left a comment

Choose a reason for hiding this comment

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

Few extra nitpicks

pyproject.toml Outdated Show resolved Hide resolved
build.py Show resolved Hide resolved
build.py Outdated Show resolved Hide resolved
@sdispater sdispater requested a review from Secrus August 8, 2023 20:19
Copy link
Collaborator

@Secrus Secrus left a comment

Choose a reason for hiding this comment

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

LGTM, great job!

@sdispater sdispater merged commit 4d86638 into master Aug 9, 2023
17 of 18 checks passed
@sdispater sdispater deleted the refactor/rust branch August 9, 2023 17:44
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