-
-
Notifications
You must be signed in to change notification settings - Fork 405
chore(prelude)!: add / remove items #1149
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
Conversation
Based on a suggestion from @BurntSushi on Reddit, this PR removes the items from the prelude that don't form a coherent common vocabulary and adds the missing items that do. > I think that in order for a prelude to be successful, it needs to form > a coherent common vocabulary. <https://www.reddit.com/r/rust/comments/1cle18j/comment/l2uuuh7/> BREAKING CHANGE: The following items have been removed from the prelude: - `style::Styled` - this trait is useful for widgets that want to support the Stylize trait, but it adds complexity as widgets have two `style` methods and a `set_style` method. - `symbols::Marker` - this item is used by code that needs to draw to the `Canvas` widget, but it's not a common item that would be used by most users of the library. - `terminal::{CompletedFrame, TerminalOptions, Viewport}` - these items are rarely used by code that needs to interact with the terminal, and they're generally only ever used once in any app. The following items have been added to the prelude: - `layout::{Position, Size}` - these items are used by code that needs to interact with the layout system. These are newer items that were added in the last few releases, which should be used more liberally.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1149 +/- ##
=====================================
Coverage 94.2% 94.2%
=====================================
Files 60 60
Lines 14509 14509
=====================================
Hits 13670 13670
Misses 839 839 ☔ View full report in Codecov by Sentry. |
#1141: The duplicate methods could also be removed to reduce confusion. Only keeping the trait methods seems like a good thing to me. |
I think Styled is the only thing where I can understand the prelude. It's a common building block to style something. Like (But even that is easily autocompleted by rust-analyzer) Personally I would look what is used across most files in a ratatui project. Backend isn't part of it, that's a one time thing in a single file → shouldn't be part of the common language. Layouting, styling, text stuff (Span, Line, …) are something that are very common. And as they are common it's easier to know, they are from ratatui. Looking at the remaining stuff… why is Masked in there? It's not a thing being used in most files. |
Backend is a trait, which means anything that uses any of the backends needs it. Masked should be removed. Let's hold the discussion of prelude use within the library a bit perhaps? Having modules in the code was an approach that simplified a lot of code when that was added. I actively don't want to make a call on that in this PR which is about removing items from the common vocabulary. |
regarding Stylize trait: I think I can be convinced it is not part of the vocabulary but it is certainly one of the more useful parts of the prelude and I would like to not remove it outright. How about putting it in |
Having multiple preludes seems even worse to me.
|
Same. As does naming this anything else other than just prelude. This is a hard push back on the idea of changing the name of the prelude from me in any way. |
Stylize is not and should not be removed from the prelude as the methods it adds everywhere are intentionally part of the vocabulary of ratatui (.bold(), .green(), etc.) This PR removes Styled (which allows Widgets to support Stylize through a blanket implementation), not Stylize. |
My bad! Ignore my comment. |
@EdJoPaTo any ideas on how to repro the test failures? They're succeeding locally for me. I'm wondering if it might be cache related. |
Merging this despite the broken tests to see if there's something that resolves in the github caching. |
They fail locally for me too. Caches can be removed and then the CI rerun to ensure it's not related to caching. |
Orhun did that and it was still the same. Edit: worked it out. Running cargo update before running the tests brings in:
|
his PR removes the items from the prelude that don't form a coherent common vocabulary and adds the missing items that do. Based on a comment at <https://www.reddit.com/r/rust/comments/1cle18j/comment/l2uuuh7/> BREAKING CHANGE: The following items have been removed from the prelude: - `style::Styled` - this trait is useful for widgets that want to support the Stylize trait, but it adds complexity as widgets have two `style` methods and a `set_style` method. - `symbols::Marker` - this item is used by code that needs to draw to the `Canvas` widget, but it's not a common item that would be used by most users of the library. - `terminal::{CompletedFrame, TerminalOptions, Viewport}` - these items are rarely used by code that needs to interact with the terminal, and they're generally only ever used once in any app. The following items have been added to the prelude: - `layout::{Position, Size}` - these items are used by code that needs to interact with the layout system. These are newer items that were added in the last few releases, which should be used more liberally.
his PR removes the items from the prelude that don't form a coherent common vocabulary and adds the missing items that do. Based on a comment at <https://www.reddit.com/r/rust/comments/1cle18j/comment/l2uuuh7/> BREAKING CHANGE: The following items have been removed from the prelude: - `style::Styled` - this trait is useful for widgets that want to support the Stylize trait, but it adds complexity as widgets have two `style` methods and a `set_style` method. - `symbols::Marker` - this item is used by code that needs to draw to the `Canvas` widget, but it's not a common item that would be used by most users of the library. - `terminal::{CompletedFrame, TerminalOptions, Viewport}` - these items are rarely used by code that needs to interact with the terminal, and they're generally only ever used once in any app. The following items have been added to the prelude: - `layout::{Position, Size}` - these items are used by code that needs to interact with the layout system. These are newer items that were added in the last few releases, which should be used more liberally.
Based on a suggestion from @BurntSushi on Reddit, this PR removes the
items from the prelude that don't form a coherent common vocabulary and
adds the missing items that do.
https://www.reddit.com/r/rust/comments/1cle18j/comment/l2uuuh7/
BREAKING CHANGE:
The following items have been removed from the prelude:
style::Styled
- this trait is useful for widgets that want tosupport the Stylize trait, but it adds complexity as widgets have two
style
methods and aset_style
method.symbols::Marker
- this item is used by code that needs to draw tothe
Canvas
widget, but it's not a common item that would be used bymost users of the library.
terminal::{CompletedFrame, TerminalOptions, Viewport}
- these itemsare rarely used by code that needs to interact with the terminal, and
they're generally only ever used once in any app.
The following items have been added to the prelude:
layout::{Position, Size}
- these items are used by code that needsto interact with the layout system. These are newer items that were
added in the last few releases, which should be used more liberally.