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(text): replace Spans with Line #178

Merged
merged 3 commits into from
May 18, 2023

Conversation

joshka
Copy link
Member

@joshka joshka commented May 17, 2023

Line is a significantly better name over Spans as the plural causes
confusion and the type really is a representation of a line of text made
up of spans.

This is a backwards compatible version of the approach from
#175

There is a significant amount of code that uses the Spans type and
methods, so instead of just renaming it, we add a new type and replace
parameters that accepts a Spans with a parameter that accepts
Into<Line>.

Note that the examples have been intentionally left using Spans in
this commit to demonstrate the compiler warnings that will be emitted in
existing code.

Implementation notes:

  • moves the Spans code to text::spans and publicly reexports on the text
    module. This makes the test in that module only relevant to the Spans
    type.
  • adds a line module with a copy of the code and tests from Spans with a
    single addition: impl<'a> From<Spans<'a>> for Line<'a>
  • adds tests for Spans (created and checked before refactoring)
  • adds the same tests for Line
  • updates all widget methods that accept and store Spans to instead
    store Line and accept Into<Line>

There is a second related commit on this PR to move text::Masked -> text::masked::Masked, with no changes.


Tagging @Eyesonjune18 and @TimerErTim for their input
This will likely require changes to be made on the changes in #149 (hopefully, just in Text and Line?)

I also updated the examples in a third commit. I wanted to make sure that backwards compatibility was maintained in a way that made sense first though so I intentionally kept the two commits separate. E.g.:

 1  warning: use of deprecated struct `ratatui::text::Spans`: Use `ratatui::text::Line` instead
   --> examples/panic.rs:29:20
    |
 29 | use ratatui::text::Spans;
    |                    ^^^^^
    |
    = note: `#[warn(deprecated)]` on by default

I think the only breaking change on this is any code that specifically calls Text::into_iter() and then stores that in a variable or field of type IntoIter<Spans<'a>> rather than just letting type inference do its thing. This seems unlikely enough that it's probably not even worth noting.

joshka added 3 commits May 17, 2023 15:12
`Line` is a significantly better name over `Spans` as the plural causes
confusion and the type really is a representation of a line of text made
up of spans.

This is a backwards compatible version of the approach from
ratatui#175

There is a significant amount of code that uses the Spans type and
methods, so instead of just renaming it, we add a new type and replace
parameters that accepts a `Spans` with a parameter that accepts
`Into<Line>`.

Note that the examples have been intentionally left using `Spans` in
this commit to demonstrate the compiler warnings that will be emitted in
existing code.

Implementation notes:
- moves the Spans code to text::spans and publicly reexports on the text
module. This makes the test in that module only relevant to the Spans
type.
- adds a line module with a copy of the code and tests from Spans with a
single addition: `impl<'a> From<Spans<'a>> for Line<'a>`
- adds tests for `Spans` (created and checked before refactoring)
- adds the same tests for `Line`
- updates all widget methods that accept and store Spans to instead
store `Line` and accept `Into<Line>`
Re-exports the Masked type at text::Masked
Copy link
Contributor

@lthoerner lthoerner left a comment

Choose a reason for hiding this comment

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

Thanks for the PR rework, LGTM.

Copy link
Contributor

@sophacles sophacles left a comment

Choose a reason for hiding this comment

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

I like where this ended up! LGTM

@sayanarijit
Copy link
Member

Lookg good 👍

Copy link
Contributor

@TimerErTim TimerErTim left a comment

Choose a reason for hiding this comment

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

LGTM
Great work and great idea to begin with!

Copy link
Member

@orhun orhun left a comment

Choose a reason for hiding this comment

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

LGTM 🦀

@orhun orhun changed the title refactor(text): replace Spans with Line refactor(text): replace Spans with Line May 18, 2023
@orhun orhun merged commit 728f82c into ratatui:main May 18, 2023
@joshka joshka deleted the refactor-rename-spans branch July 9, 2023 02:18
lusingander added a commit to lusingander/stu that referenced this pull request Sep 13, 2023
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.

6 participants