Skip to content

Refactor arithmetic tests to improve tooling support #7552

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.

Sign up for GitHub

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

Jump to bottom
Open
mega123-art wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Refactor arithmetic tests to improve tooling support #7552

mega123-art wants to merge 8 commits into from
+110 −106

Conversation

@mega123-art
Copy link
Contributor

@mega123-art mega123-art commented Feb 3, 2026 •
edited
Loading

This pr adds tooling as suggested by @sffc

mega123-art and others added 6 commits January 30, 2026 07:21
@mega123-art
test(calendar): Add comprehensive date arithmetic coverage tests
b9b3fa3 
Implementation of comprehensive date arithmetic coverage tests as suggested in unicode-org#7215.
@mega123-art
Merge branch 'main' into test/calendar-coverage
1c879d8
@mega123-art
Merge branch 'main' into test/calendar-coverage
7d1e6a2
@mega123-art @robertbastian
Apply suggestion from @robertbastian
ddce0c8 
Co-authored-by: Robert Bastian <4706271+robertbastian@users.noreply.github.com>
@mega123-art @robertbastian
Update components/calendar/src/tests/arithmetic_exhaustive.rs
a22ea51 
Co-authored-by: Robert Bastian <4706271+robertbastian@users.noreply.github.com>
@mega123-art
Refactor test_arithmetic to move implementation outside macro
383d4e5 
As suggested by @sffc, this improves IDE support, formatting, and
rust-analyzer functionality by keeping the test logic outside the
macro expansion.
@robertbastian
Copy link
Member

robertbastian commented Feb 3, 2026

I don't think this is an improvement, the IDE issue that @sffc mentioned is non-existent, and this adds an extra level of redirection that makes it harder to follow the test

@mega123-art
Copy link
Contributor Author

mega123-art commented Feb 3, 2026

I don't think this is an improvement, the IDE issue that @sffc mentioned is non-existent, and this adds an extra level of redirection that makes it harder to follow the test

Ig u r right lets see what @sffc tells

@Manishearth Manishearth removed their request for review February 3, 2026 21:21
@sffc
Copy link
Member

sffc commented Feb 5, 2026 •
edited
Loading

Thanks! But, there is no deleted code. This is currently adding a new test without replacing an existing test.

Fixed via merge.

sffc
sffc approved these changes Feb 5, 2026
View reviewed changes
Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

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

thanks!

robertbastian
robertbastian requested changes Feb 8, 2026
View reviewed changes
Copy link
Member

@robertbastian robertbastian left a comment

Choose a reason for hiding this comment

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

the test_all_cals! macros is written in such a way that you make it wrap your test function. this is how all its invocations in this crate are written. this PR makes code paths harder to follow

@sffc
Copy link
Member

sffc commented Feb 9, 2026

I checked and it seems cargo fmt works now in this type of macro invocation. I'm fairly certain it didn't always work. Haven't checked rust-analyzer.

I still find the refactor better since it reduces a level of indentation. I don't find it harder to follow the code path.

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

Reviewers

@robertbastian robertbastian robertbastian requested changes

@sffc sffc sffc approved these changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mega123-art @robertbastian @sffc