8000 chore(prelude)!: add / remove items by joshka · Pull Request #1149 · ratatui/ratatui · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

Merged
merged 12 commits into from
Jun 5, 2024
Merged

chore(prelude)!: add / remove items #1149

merged 12 commits into from
Jun 5, 2024

Conversation

joshka
Copy link
Member
@joshka joshka commented May 27, 2024

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.

@github-actions github-actions bot added the Type: Breaking Change This change will cause application code to break and must be noted in the breaking changes docs etc. label May 27, 2024
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.
@joshka joshka force-pushed the jm/prelude-vocab branch from eec309f to aa2a6f9 Compare May 27, 2024 04:02
Copy link
codecov bot commented May 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.2%. Comparing base (d7ed6c8) to head (16e2e14).
Report is 2 commits behind head on main.

Current head 16e2e14 differs from pull request most recent head 745c37a

Please upload reports for the commit 745c37a to get more accurate results.

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.
📢 Have feedback on the report? Share it here.

@EdJoPaTo
Copy link
Contributor
  • 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.

#1141: The duplicate methods could also be removed to reduce confusion. Only keeping the trait methods seems like a good thing to me.

@EdJoPaTo
Copy link
Contributor

I think Styled is the only thing where I can understand the prelude. It's a common building block to style something. Like ToString is part of the std prelude.

(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. symbols also seem somewhat weird. Having whole modules in there in general seems weird too.

@joshka
Copy link
Member Author
joshka commented May 27, 2024

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.

@kdheepak
Copy link
Collaborator
kdheepak commented May 27, 2024

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 inuse ratatui::prelude::traits::* along with all other traits? If that happens that will be the only prelude that ends up in the final version of all my projects.

@EdJoPaTo
Copy link
Contributor

How about putting it inuse ratatui::prelude::traits::* along with all other traits? If that happens that will be the only prelude that ends up in the final version of all my projects.

Having multiple preludes seems even worse to me.

clippy::wildcard_imports explicitly ignores wildcard imports from modules containing the name prelude, so ratatui::trait_prelude::* would be a better fit to work together with this lint.
(I think wildcards are similarly annoying for readability and avoid them like I avoid preludes, especially as rust-analyzer can automatically add stuff anyway.)

@joshka
Copy link
Member Author
joshka commented May 27, 2024

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.

@joshka
Copy link
Member Author
joshka commented May 27, 2024

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.

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.

@kdheepak
Copy link
Collaborator

My bad! Ignore my comment.

@joshka joshka requested a review from EdJoPaTo June 1, 2024 10:36
@joshka joshka changed the title chore(prelude)!: Remove uncommon items from the prelude chore(prelude)!: Adjust prelude to more common items Jun 4, 2024
@joshka joshka changed the title chore(prelude)!: Adjust prelude to more common items chore(prelude)!: add / remove items Jun 4, 2024
@joshka joshka requested a review from EdJoPaTo June 4, 2024 21:42
@joshka
Copy link
Member Author
joshka commented Jun 4, 2024

@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.

@joshka
Copy link
Member Author
joshka commented Jun 5, 2024

Merging this despite the broken tests to see if there's something that resolves in the github caching.

@joshka joshka merged commit 7b45f74 into main Jun 5, 2024
14 of 31 checks passed
@joshka joshka deleted the jm/prelude-vocab branch June 5, 2024 03:59
@EdJoPaTo
Copy link
Contributor
EdJoPaTo commented Jun 5, 2024

They fail locally for me too.

Caches can be removed and then the CI rerun to ensure it's not related to caching.

@joshka
Copy link
Member Author
joshka commented Jun 5, 2024

Orhun did that and it was still the same.
Locally, I've run cargo clean, updated rustup, stopped sccache and restarted it, and whatever I do I'm seeing success on the failing tests.

Edit: worked it out. Running cargo update before running the tests brings in:

❯ cargo update
    Updating crates.io index
    Updating strum_macros v0.26.3 -> v0.26.4
    Updating unicode-width v0.1.12 -> v0.1.13
note: pass `--verbose` to see 60 unchanged dependencies behind latest

#1170
unicode-rs/unicode-width#55

itsjunetime pushed a commit to itsjunetime/ratatui that referenced this pull request Jun 23, 2024
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.
joshka added a commit to erak/ratatui that referenced this pull request Oct 14, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Breaking Change This change will cause application code to break and must be noted in the breaking changes docs etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0