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(spans): remove deprecated Spans type #426

Merged
merged 1 commit into from
Sep 19, 2023
Merged

Conversation

joshka
Copy link
Member

@joshka joshka commented Aug 24, 2023

Spans was replaced with Line in 0.21.0. This commit removes the
deprecated type and all associated code. Buffer::set_spans was
replaced with Buffer::set_line.

@codecov
Copy link

codecov bot commented Aug 24, 2023

Codecov Report

Merging #426 (45f8a67) into main (dd9a8df) will increase coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #426      +/-   ##
==========================================
+ Coverage   89.93%   89.98%   +0.04%     
==========================================
  Files          41       40       -1     
  Lines       11334    11160     -174     
==========================================
- Hits        10193    10042     -151     
+ Misses       1141     1118      -23     
Files Changed Coverage Δ
src/buffer.rs 96.72% <ø> (+2.65%) ⬆️
src/text/line.rs 96.31% <ø> (-0.24%) ⬇️
src/text/text.rs 100.00% <ø> (ø)

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.

Haven't tested the code but general +1

@joshka
Copy link
Member Author

joshka commented Aug 24, 2023

The main question on this is whether it's for 0.23 or 0.24? It's been 3 months since the deprecation, which is probably enough time to handle this sort of thing.

@kdheepak
Copy link
Collaborator

kdheepak commented Aug 25, 2023

Looking at https://crates.io/crates/ratatui/reverse_dependencies there's still quite a few projects still on 0.20.1. I'm assuming they wouldn't even see the deprecation warning.

That said, I'm assuming that those on v0.20.1 are probably only going to upgrade when there's a feature they want. So it's probably fine to go for it in this release.

@joshka
Copy link
Member Author

joshka commented Aug 25, 2023

That's actually a reasonable check - I think perhaps it's worth holding off until after 0.23 then.

@joshka joshka added Status: Blocked An issue or PR that cannot be completed until some other task is complete Status: Do Not Merge Prevent a PR from being merged. Usually indicates something that should be held to a future release labels Aug 25, 2023
@joshka joshka added this to the v0.24.0 milestone Aug 25, 2023
@mindoodoo
Copy link
Member

Should we maybe add mention somewhere of how long we keep things in a deprecated state before we remove them ? Do we already have that and I missed it ?

Consistency in our deprecation process would be good to have for sure

@joshka
Copy link
Member Author

joshka commented Sep 12, 2023

Deprecation should be rare enough that we can mostly handle it on a case by case basis IMO.

Latest push is a rebase and removed a couple of tests that referenced Spans.

I think this is probably worth merging for 0.24

@joshka joshka removed Status: Blocked An issue or PR that cannot be completed until some other task is complete Status: Do Not Merge Prevent a PR from being merged. Usually indicates something that should be held to a future release labels Sep 13, 2023
- `Line` replaces `Spans`
- `Buffer::set_line` replaces `Buffer::set_spans`
@joshka
Copy link
Member Author

joshka commented Sep 17, 2023

Rebased on main to resolve conflicts (changes from mod.rs -> xxx.rs)

@joshka joshka merged commit 5498a88 into ratatui:main Sep 19, 2023
33 checks passed
@joshka joshka deleted the remove-spans branch September 19, 2023 09:58
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.

4 participants