fix(block and several others): deprecate title struct, it's position, and related functions #1061
Conversation
TadoTheMiner
requested review from
orhun,
mindoodoo,
sayanarijit,
joshka,
kdheepak,
Valentin271 and
EdJoPaTo
as code owners
TadoTheMiner
force-pushed
the
deprecate_title
branch
from
fd8edfc
to
2563e09
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1061 /- ##
=======================================
- Coverage 94.3% 94.2% -0.1%
=======================================
Files 61 61
Lines 14768 14770 2
=======================================
- Hits 13932 13922 -10
- Misses 836 848 12 ☔ View full report in Codecov by Sentry. |
… and related functions
TadoTheMiner
force-pushed
the
deprecate_title
branch
from
ed89a4f
to
4a9e1b6
Compare
|
I've added fn foo(position: Position) -> Block {
Block::new().title(Title::from("bar").position(position))
}would have to write their own enum since |
|
Forgot mention issue #738 |
There was a problem hiding this comment.
First up, I want to thank you for your effort in doing this. I know it's been a PR that's been deprioritized for some time. Unfortunately, it's still significantly too large to review in this form and from looking at the commits there seem to be too many different changes as well. I suspect this is the result of a bad merge, but I'm not 100% sure. It has also suffered as being lower on the list due to the value vs difficulty measure being out of proportion (this means it takes a lot of motivation to dig in and understand the context of the solution, and so personally that mountain is hard to overcome).
I'd suggest taking a few steps back and looking at the requirements and how they may be achieved in small steps. This may require having to redo some changes. Unfortunately I haven't yet done that myself as this was something on the someday backlog and it had a large amount of context necessary to understand where we're at with this.
Trying to change multiple things at once leads to an exponential amount of review effort and possible ways to fail. A good goal for a PR is somewhere closer to 200LoC as a maximum (or more when they are strictly refactoring only changes that are repetitive and easy to confirm as being the same).
This might mean that this PR needs to be redone or even abandoned if we talk about what needs to happen and decide differently. If that is what happens, I want to thank you again for trying to help with this.
| @@ -10,39 10,19 @@ This folder might use unreleased code. View the examples for the latest release | |||
| > There are a few workaround for this problem: | |||
| > | |||
| > - View the examples as they were when the latest version was release by selecting the tag that | |||
| > matches that version. E.g. <https://github.com/ratatui/ratatui/tree/v0.26.1/examples>. | |||
| > matches that version. E.g. <https://github.com/ratatui-org/ratatui/tree/v0.26.1/examples>. | |||
There was a problem hiding this comment.
I think there's an issue with this PR because it "regresses" a number of changes to the code.
Here's the simplest example - ratatui-org was renamed to ratatui and this PR undoes that renaming. I'm seeing a lot of changes that undoes this renaming, changes that undoes typo fixes etc, changes that undoes documentation additions etc.
It's hard to make out the exact diff associated with this particular change.
|
I'm not sure if #1372 is what this did, due to the merge issues, but if it is, then this can be closed |
I've deprecated
Title, thetitle()function,Position, and thetitles_position()function, and adapted the docs to havetitle_top()andtitle_bottom()in examples. While it was suggested to addtop_title()andbottom_title(), and deprecate the old ones, there's no need becausetitle_top()andtitle_bottom()have the same signatures, as the new functions would. It was also suggested that we add thetop_titlesandbottom_titlesfields, this is currently not possible since thetitle()function supports having titles with no position. So we should rather add the fields after removing the deprecations. I was forced to add#![cfg_attr(test, allow(deprecated)]tolib.rsdue to a rust bug. And you can ignoreflake.nixandflake.lock, it's for Nix users, as I've switched to NixOS recently.