ratatui / ratatui-macros

Macros for simplifying boilerplate for creating UI using Ratatui.
https://ratatui.rs
MIT License
24 stars 2 forks source link

feat: Use `::ratatui` instead of `ratatui` #54

Closed kdheepak closed 2 months ago

kdheepak commented 2 months ago

This PR uses ::ratatui to ensure it doesn't clash with a module named ratatui in an app, per comment https://github.com/ratatui-org/ratatui-macros/pull/52#discussion_r1656670148

This PR also refactors some tests.

kdheepak commented 2 months ago

Should this be a breaking change?

joshka commented 2 months ago

Should this be a breaking change?

no

kdheepak commented 2 months ago

Can you elaborate on why this is not a breaking change? Just trying to understand.

Currently, if a macro like this is defined in this crate:

#[macro_export]
macro_rules! my_custom_macro {
    () => {
        foobar::test()
    };
}

Even if the foobar is not defined in this crate, this crate will compile fine. Then, a user can use this macro by defining a foobar module in their code and calling the macro:

mod foobar {
    pub fn test() -> String {
        String::default()
    }
}

fn test_function() {
  let s = my_custom_macro!();
}

i.e. the current behavior is to call functions from the ratatui module in the macro expansion scope or calling scope, or the ratatui dependency that they choose to add into their project.

But with this PR, the behavior is to call functions from the ::ratatui module that is a dependency of the ratatui-macros crate.

So technically this could be breaking, right?

joshka commented 2 months ago

Can you elaborate on why this is not a breaking change? Just trying to understand.

Currently, if a macro like this is defined in this crate:

#[macro_export]
macro_rules! my_custom_macro {
    () => {
        foobar::test()
    };
}

Even if the foobar is not defined in this crate, this crate will compile fine. Then, a user can use this macro by defining a foobar module in their code and calling the macro:

mod foobar {
    pub fn test() -> String {
        String::default()
    }
}

fn test_function() {
  let s = my_custom_macro!();
}

i.e. the current behavior is to call functions from the ratatui module in the macro expansion scope or calling scope, or the ratatui dependency that they choose to add into their project.

But with this PR, the behavior is to call functions from the ::ratatui module that is a dependency of the ratatui-macros crate.

So technically this could be breaking, right?

Technically yes, but this is the opposite of Unicode-width crate mess that wasn't a "breaking change" despite it being a "change that broke things". This is a "breaking change" that "doesn't break anything". There's 19 results in https://github.com/search?q=ratatui_macros&type=code and none of them define a ratatui module in their code that would be impacted by this. I wouldn't sweat this.

EdJoPaTo commented 2 months ago

A breaking change is when the intended behaviour changes. This PR ensures that the intended behaviour always works. It’s a fix to ensure it actually works as it should. That’s the same of what unicode-width did: ensuring that it does what it should.


This PR seems to also include unrelated changes?

kdheepak commented 2 months ago

This PR seems to also include unrelated changes?

It was refactoring of the tests, I guess that should have gone in a separate PR?

joshka commented 2 months ago

Not a big deal in this lib.