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(list)!: remove deprecated start_corner and Corner #759

Merged
merged 3 commits into from May 24, 2024

Conversation

Valentin271
Copy link
Member

@Valentin271 Valentin271 commented Jan 6, 2024

List::start_corner was deprecated in v0.25. Use List::direction and ListDirection instead.

- list.start_corner(Corner::TopLeft);
- list.start_corner(Corner::TopRight);
// This is not an error, BottomRight rendered top to bottom previously
- list.start_corner(Corner::BottomRight);
// all becomes
+ list.direction(ListDirection::TopToBottom);
- list.start_corner(Corner::BottomLeft);
// becomes
+ list.direction(ListDirection::BottomToTop);

layout::Corner is removed entirely.

@joshka
Copy link
Member

joshka commented Jan 6, 2024

Perhaps we could let this one live for a minute? There's not a huge amount of users, but 0.25 is the most recent release.
https://github.com/search?q=ratatui+start_corner+AND+%28NOT+org%3Aratatui-org%29+AND+%28NOT+is%3Afork%29&type=code

@Valentin271
Copy link
Member Author

I think one version is enough for an easy fix like that. But I don't mind holding it for a while.

Let's see what the others have to say.

Maybe we should state in the contributing guide how much we want to wait before removing deprecated items.

@Valentin271
Copy link
Member Author

To add to my previous comment:

When the deprecated feature was clear and didn't have problem in itself, I agree to keep it a few version.
But when it's something that wasn't clear how to use or its effects, I think basically people won't miss it because they did not understand what it was in the first place.

@joshka
Copy link
Member

joshka commented Jan 6, 2024

I think 2 releases is a good idea generally. This gives our users enough time to update to a release, test on that release and have users test it for a bit. We're releasing Ratatui about every 6-8 weeks, so a 2 version deprecation gives 3 months notice of breaking changes.

There might be a few changes that we want to move users away from more forcefully so we can implement a new feature where the 2 release approach doesn't work, but for things like this, where the downside of keeping it is only that the API is messy, I'm fine with it.

^Weak opinion - if you do feel more strongly than I do about removing this sooner, then I'm happy to support that.

@Valentin271
Copy link
Member Author

Weak opinion for me too, It's literally that I don't like keeping a messy API for too long.

So conclusion for me:

  • If a deprecation is blocking for us to implement a new feature we may consider removing it in a one version notice
  • Otherwise if the deprecation is not blocking for anything, wait at least 2 version

@Valentin271
Copy link
Member Author

Updated CONTRIBUTING.md

#761

@orhun
Copy link
Sponsor Member

orhun commented Jan 7, 2024

+1 from me, I think having a 2 release notice is good enough for removing deprecated items. Let's also add these types of issues to the future milestones (which I saw it is already added!).

@joshka
Copy link
Member

joshka commented Jan 30, 2024

Let's merge this right before 0.27.0 releases to avoid putting breaking changes in the repo right after release. Marking it draft until then.

@joshka joshka marked this pull request as draft January 30, 2024 07:06
`List::start_corner` was deprecated in v0.25. Use `List::direction` and `ListDirection` instead.
@Valentin271 Valentin271 marked this pull request as ready for review February 12, 2024 21:40
@Valentin271
Copy link
Member Author

Rebased on main. It is unlikely we will release 0.26.2 in my opinion.

@joshka
Copy link
Member

joshka commented Feb 12, 2024

Rebased on main. It is unlikely we will release 0.26.2 in my opinion.

Probably not, but it's also not a huge thing to hold back from merging. Merging this means that we can't do 26.2 if needed.
I'd still caution about merging this until we're closer to a major release.

@Valentin271
Copy link
Member Author

Works for me, was easy to rebase and probably still will be later on. We can wait.

@joshka joshka requested a review from EdJoPaTo as a code owner May 24, 2024 05:44
Copy link

codecov bot commented May 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.9%. Comparing base (257db62) to head (08f796b).
Report is 1 commits behind head on main.

Current head 08f796b differs from pull request most recent head c888bb8

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

Additional details and impacted files
@@           Coverage Diff           @@
##            main    #759     +/-   ##
=======================================
- Coverage   94.3%   91.9%   -2.4%     
=======================================
  Files         61      61             
  Lines      14767   15668    +901     
=======================================
+ Hits       13926   14401    +475     
- Misses       841    1267    +426     

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

@EdJoPaTo
Copy link
Member

Is Corner used somewhere else? Otherwise, it can be removed too.

Copy link
Sponsor 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.

needs a CI fix, otherwise LGTM

@joshka
Copy link
Member

joshka commented May 24, 2024

Is Corner used somewhere else? Otherwise, it can be removed too.

Nope - removing.

@joshka joshka changed the title refactor(list)!: remove deprecated start_corner refactor(list)!: remove deprecated start_corner and Corner May 24, 2024
@joshka joshka merged commit 4770e71 into ratatui-org:main May 24, 2024
36 of 37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants