Skip to content

fix: Don't include modules via a macro #935

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

Merged
merged 1 commit into from
May 8, 2025

Conversation

nicklan
Copy link
Collaborator

@nicklan nicklan commented May 8, 2025

What changes are proposed in this pull request?

Turns out rustfmt won't format module files that are included via a macro. See here.

This meant things like log_segment.rs that were included via internal_mod! were not getting formatted.

This removes that macro and just uses the "double include" gated by feature flags, which rustfmt can handle.

How was this change tested?

Making sure that miss-formatted code in the effected modules is now formatted when running cargo fmt.

Copy link
Collaborator

@zachschuermann zachschuermann left a comment

Choose a reason for hiding this comment

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

yay thanks! good find - sorry i missed that when I originally did that macro

@zachschuermann
Copy link
Collaborator

(and MSRV is known failure right now due to icu bumping up to 1.82)

Copy link

codecov bot commented May 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.84%. Comparing base (a67f4d0) to head (5c4faf5).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #935   +/-   ##
=======================================
  Coverage   84.84%   84.84%           
=======================================
  Files          88       88           
  Lines       22766    22766           
  Branches    22766    22766           
=======================================
  Hits        19316    19316           
  Misses       2446     2446           
  Partials     1004     1004           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nicklan
Copy link
Collaborator Author

nicklan commented May 8, 2025

yay thanks! good find - sorry i missed that when I originally did that macro

No worries, it's a really odd edge case

@github-actions github-actions bot added the breaking-change Change that require a major version bump label May 8, 2025
@nicklan nicklan removed the breaking-change Change that require a major version bump label May 8, 2025
@nicklan
Copy link
Collaborator Author

nicklan commented May 8, 2025

(removing breaking_change label as I don't believe this is actually a breaking change. it just sees we removed an exported macro, but no one is using it)

@nicklan nicklan merged commit a4993ff into delta-io:main May 8, 2025
20 of 22 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.

3 participants