ratatui-org / ratatui

Rust library that's all about cooking up terminal user interfaces (TUIs) 👨‍🍳🐀
https://ratatui.rs
MIT License
8.86k stars 269 forks source link

`clippy::same_name_method` with Stylize #1141

Open EdJoPaTo opened 1 month ago

EdJoPaTo commented 1 month ago

Problem

Stylize introduces style and set_style. They are also implemented manually too. clippy::same_name_method notices this.

Solution

Probably remove the manually implemented methods and only use the ones implemented on Stylize. This will be a breaking change as the trait needs to be imported then.

joshka commented 1 month ago

The problem is the styled trait is much newer than all the ratatui apps in existence. This would break every file that has ratatui code in every one of them as they'd have to change every call to .style() to .set_style() and add the trait. The size of this break is too large to consider as being a reasonable thing even with deprecation and such.

joshka commented 2 weeks ago

To move this ahead, I'd suggest thinking through the impact of this change on downstream code. It would be useful to consider the specific magnitude (how much will be affected) and effort involved in updating the code (i.e. specifically what would the compiler do, what find and replace would help, etc.). I've assumed above that these are likely too large to consider, and this might be correct, but perhaps there's an incremental approach that I'm missing here.

It might also be worth thinking about possible alternatives to this. Something that I've been thinking about is whether it's worth implementing the stylize methods directly on widgets using either declarative or derive macros rather than a blanket trait implementation. There are likely downsides to this approach too.

EdJoPaTo commented 2 weeks ago

Macros would be neat as that can result in const methods then. As styles are done many times per render that could be a performance benefit on runtime, but comes at a compile time cost of these macros and their own complexity of writing / maintaining them. I don't think that its worth the effort here.

rust-analyzer should suggest importing traits for used methods so writing them newly shouldn't be a big problem for people using rust-analyzer. Migrating from direct impl to trait seems like a rather simple change, just add the import. Noting that in the breaking changes section should be easy to adapt to. Also, the Rust error output suggests importing the trait too if I remember correctly. Should be checked.

joshka commented 2 weeks ago

Macros would be neat as that can result in const methods then. As styles are done many times per render that could be a performance benefit on runtime, but comes at a compile time cost of these macros and their own complexity of writing / maintaining them. I don't think that its worth the effort here.

I'd be surprised if the performance gains / losses mentioned are meaningful in any reasonable use case.

We use macros for the colors and modifiers already, so the maintenance burden isn't high, and the existing implementation is simple and clear on this already. Maintenance burden would be unchanged.

rust-analyzer should suggest importing traits for used methods so writing them newly shouldn't be a big problem for people using rust-analyzer.

New users regularly have issues copying code and where trait imports are necessary.

Migrating from direct impl to trait seems like a rather simple change, just add the import. Noting that in the breaking changes section should be easy to adapt to. Also, the Rust error output suggests importing the trait too if I remember correctly. Should be checked.

The quantity of affected code is relevant here. When you change something that effects every single application built with ratatui like this, think of the time necessary to understand, read, fix, commit, review, and deploy this change for every app.

joshka commented 2 weeks ago

I'd be surprised if the performance gains / losses mentioned are meaningful in any reasonable use case.

Confirmed there is no meaningful difference: