Skip to content

Rewrite extensions in Rust - #721

Merged
sdispater merged 19 commits into
masterfrom
refactor/rust
Aug 9, 2023
Merged

Rewrite extensions in Rust#721
sdispater merged 19 commits into
masterfrom
refactor/rust

Conversation

@sdispater

@sdispater sdispater commented Jul 18, 2023

Copy link
Copy Markdown
Collaborator

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

codspeed-hq Bot commented Jul 18, 2023

Copy link
Copy Markdown

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

@Secrus Secrus left a comment

Copy link
Copy Markdown
Collaborator

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.

Comment thread .github/workflows/tests.yml
Comment thread Cargo.toml
Comment thread Makefile Outdated
Comment thread build.py
Comment thread pendulum/helpers.py
Comment thread pendulum/parser.py Outdated
Comment thread pyproject.toml Outdated
Comment thread pyproject.toml Outdated
Comment thread rust/helpers.rs
@Secrus Secrus mentioned this pull request Aug 5, 2023
2 tasks

@Maneren Maneren left a comment

Copy link
Copy Markdown

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.

Comment thread rust/parsing.rs Outdated
Comment thread rust/parsing.rs Outdated
Comment thread rust/parsing.rs Outdated
Comment thread rust/parsing.rs Outdated
Comment thread rust/parsing.rs Outdated
Comment thread rust/python/types/timezone.rs Outdated
Comment thread rust/parsing.rs
Comment thread rust/parsing.rs Outdated
Comment thread rust/parsing.rs Outdated
Comment thread rust/parsing.rs Outdated
@sdispater

Copy link
Copy Markdown
Collaborator Author

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

@Maneren

Maneren commented Aug 6, 2023

Copy link
Copy Markdown

Happy to help

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

@Secrus Secrus left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Few extra nitpicks

Comment thread pyproject.toml Outdated
Comment thread build.py
Comment thread build.py Outdated
@sdispater
sdispater requested a review from Secrus August 8, 2023 20:19

@Secrus Secrus left a comment

Copy link
Copy Markdown
Collaborator

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