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

chore: reduce proc-macro dep strum #169

Merged
merged 2 commits into from
Jan 29, 2025
Merged

Conversation

tisonkun
Copy link
Contributor

Get rid of proc-macro deps for:

  • Simple dependencies
  • No need to execute proc-macro (it takes some extra compile time)

@Nukesor this patch may be more like a personal preference, so it's up to you to accept. Typically, if the TableComponent is stable, we don't need the EnumIter for convenience but just finalize it.

Otherwise, I'd prefer use enum-iterator who has no transitive deps and do exactly the same thing.

Copy link
Owner

@Nukesor Nukesor left a comment

Choose a reason for hiding this comment

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

If comfy-table wouldn't have such a good test coverage, I would actually advise against such a change.

Manually implemented iterators for enums are usually a bad idea as it's really easy to forget to adjust them if new variants are added. And there's no way to throw a lint error for stuff like this.

But as you said, that enum should be relatively stable and even if something would change, we would notice via the test coverage, so in this case I think this is a neat change :)

src/style/table.rs Outdated Show resolved Hide resolved
Signed-off-by: tison <[email protected]>
Comment on lines +71 to +93
const fn components() -> [TableComponent; 19] {
[
TableComponent::LeftBorder,
TableComponent::RightBorder,
TableComponent::TopBorder,
TableComponent::BottomBorder,
TableComponent::LeftHeaderIntersection,
TableComponent::HeaderLines,
TableComponent::MiddleHeaderIntersections,
TableComponent::RightHeaderIntersection,
TableComponent::VerticalLines,
TableComponent::HorizontalLines,
TableComponent::MiddleIntersections,
TableComponent::LeftBorderIntersections,
TableComponent::RightBorderIntersections,
TableComponent::TopBorderIntersections,
TableComponent::BottomBorderIntersections,
TableComponent::TopLeftCorner,
TableComponent::TopRightCorner,
TableComponent::BottomLeftCorner,
TableComponent::BottomRightCorner,
]
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Manually implemented iterators for enums are usually a bad idea as it's really easy to forget to adjust them if new variants are added. And there's no way to throw a lint error for stuff like this.

Yes. This is the original reason we use a macro to generate.

I try to use an internal method return [TableComponent; 19] so that the compiler can help a bit.

@tisonkun tisonkun requested a review from Nukesor January 29, 2025 03:28
Copy link

codecov bot commented Jan 29, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.59%. Comparing base (a1cf06f) to head (6168f9f).
Report is 17 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #169      +/-   ##
==========================================
- Coverage   96.75%   96.59%   -0.16%     
==========================================
  Files          16       16              
  Lines        1754     1793      +39     
==========================================
+ Hits         1697     1732      +35     
- Misses         57       61       +4     

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

@tisonkun
Copy link
Contributor Author

Seems the CI failure unrelated?

@Nukesor
Copy link
Owner

Nukesor commented Jan 29, 2025

Yeah. Rust toolchain for wasm32-wasi is broken again :D

@Nukesor Nukesor merged commit 19e29c5 into Nukesor:main Jan 29, 2025
12 of 14 checks passed
@Nukesor
Copy link
Owner

Nukesor commented Jan 29, 2025

Thx for the contribution :)

@tisonkun tisonkun deleted the reduce-deps branch January 29, 2025 09:51
@tisonkun
Copy link
Contributor Author

@Nukesor thanks for creating such a good crate! I've successfully use it in generating the table format output of our database :D

Do you have a plan for cutting a release So that I can bump in the application?

@Nukesor
Copy link
Owner

Nukesor commented Jan 29, 2025

There're a few critical bug fixes that I want to include in the next release.

I'm not sure when I'll find the time to address those.

@tisonkun
Copy link
Contributor Author

There're a few critical bug fixes that I want to include in the next release.

Is there a traking issue or your can list them here? Perhaps I can spend some time on it but I can't promise it :P

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.

2 participants