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

feat: Use ::ratatui instead of ratatui #54

Merged
merged 4 commits into from
Jun 28, 2024
Merged

feat: Use ::ratatui instead of ratatui #54

merged 4 commits into from
Jun 28, 2024

Conversation

kdheepak
Copy link
Collaborator

@kdheepak kdheepak commented Jun 28, 2024

This PR uses ::ratatui to ensure it doesn't clash with a module named ratatui in an app, per comment #52 (comment)

This PR also refactors some tests.

@kdheepak kdheepak requested review from joshka and EdJoPaTo June 28, 2024 04:25
@kdheepak kdheepak changed the title feat: Better tests feat: Use ::ratatui instead of ratatui Jun 28, 2024
@kdheepak
Copy link
Collaborator Author

Should this be a breaking change?

@joshka
Copy link
Member

joshka commented Jun 28, 2024

Should this be a breaking change?

no

@kdheepak
Copy link
Collaborator Author

kdheepak commented Jun 28, 2024

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
Copy link
Member

joshka commented Jun 28, 2024

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
Copy link
Contributor

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 kdheepak merged commit 8913e2c into main Jun 28, 2024
3 checks passed
@kdheepak kdheepak deleted the kd/feedback branch June 28, 2024 13:16
@github-actions github-actions bot mentioned this pull request Jun 28, 2024
@kdheepak
Copy link
Collaborator Author

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
Copy link
Member

joshka commented Jun 28, 2024

Not a big deal in this lib.

kdheepak pushed a commit that referenced this pull request Jun 29, 2024
## 🤖 New release
* `ratatui-macros`: 0.4.1 -> 0.4.2

<details><summary><i><b>Changelog</b></i></summary><p>

<blockquote>

##
[0.4.2](v0.4.1...v0.4.2)
- 2024-06-29

### Added
- Use `::ratatui` instead of `ratatui`
([#54](#54))
- Add row! macro
([#52](#52))

### Other
- Update README with row! documentation
([#56](#56))
- Make doc examples shorter by removing duplicate imports
([#55](#55))
</blockquote>


</p></details>

---
This PR was generated with
[release-plz](https://github.com/MarcoIeni/release-plz/).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

3 participants