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

refactor: remove pointless default on internal structs #980

Merged
merged 4 commits into from Mar 12, 2024

Conversation

EdJoPaTo
Copy link
Member

@EdJoPaTo EdJoPaTo commented Mar 4, 2024

See #978

Also remove other derives. They are unused and just slow down compilation.

also remove other derives. They are unused and just slow down compilation.
Prefer None over some useless default
Copy link

codecov bot commented Mar 4, 2024

Codecov Report

Attention: Patch coverage is 86.95652% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 92.0%. Comparing base (c4ce7e8) to head (5fc5b28).
Report is 2 commits behind head on main.

Files Patch % Lines
src/widgets/canvas.rs 0.0% 4 Missing ⚠️
src/widgets/chart.rs 95.2% 2 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main    #980   +/-   ##
=====================================
  Coverage   92.0%   92.0%           
=====================================
  Files         61      61           
  Lines      15933   15937    +4     
=====================================
+ Hits       14660   14668    +8     
+ Misses      1273    1269    -4     

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

Copy link
Member

@Valentin271 Valentin271 left a comment

Choose a reason for hiding this comment

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

Hey, this looks good to me, thanks for this.

Had a little comment but it's non-blocking.

src/widgets/chart.rs Outdated Show resolved Hide resolved
The abort should never be triggered as long as the code above is correct. An assertion checking the code above is correct is more useful then.
@EdJoPaTo
Copy link
Member Author

EdJoPaTo commented Mar 6, 2024

It might be a good idea to split the canvas changes from the rest of this PR as it has also changes to the code behavior?

@Valentin271
Copy link
Member

Surely I'm forgetting/misunderstanding something... Canvas doesn't have a change in behavior ..?

@EdJoPaTo
Copy link
Member Author

EdJoPaTo commented Mar 7, 2024

Canvas doesn't have a change in behavior ..?

The checks and math logic has changed. That might have changed something. It's not much so it could be fine to mix it.

@joshka
Copy link
Member

joshka commented Mar 12, 2024

The checks and math logic has changed. That might have changed something. It's not much so it could be fine to mix it.

I think you might be referring to the Chart (not canvas)
If it didn't break the tests (and those lines are covered by tests) then I'm not bothered too much.

@joshka joshka merged commit 8e68db9 into ratatui-org:main Mar 12, 2024
32 of 33 checks passed
@EdJoPaTo EdJoPaTo deleted the useless-default-internal branch March 12, 2024 09:22
joshka pushed a commit to nowNick/ratatui that referenced this pull request May 24, 2024
See ratatui-org#978

Also remove other derives. They are unused and just slow down
compilation.
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.

None yet

4 participants