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

fix(block)!: remove title #1012

Closed
wants to merge 7 commits into from

Conversation

TadoTheMiner
Copy link
Contributor

@TadoTheMiner TadoTheMiner commented Apr 1, 2024

This PR is a draft. Firstly, it's a breaking change, secondly, a test fails, because for some reason instead of None the line has Some(Reset) in block_title_style. I couldn't find out what causes that, but I will try to address that issue.

BREAKING_CHANGE: use line instead

@TadoTheMiner TadoTheMiner changed the title Remove block::title fix(title, block) removed title Apr 1, 2024
@TadoTheMiner TadoTheMiner changed the title fix(title, block) removed title fix(title, block): removed title Apr 1, 2024
@TadoTheMiner TadoTheMiner marked this pull request as draft April 1, 2024 14:45
@TadoTheMiner TadoTheMiner changed the title fix(title, block): removed title fix(title, block): removed title BREAKING_CHANGE: Apr 1, 2024
@TadoTheMiner TadoTheMiner changed the title fix(title, block): removed title BREAKING_CHANGE: fix(title, block)!: removed title BREAKING_CHANGE: Apr 1, 2024
@TadoTheMiner TadoTheMiner changed the title fix(title, block)!: removed title BREAKING_CHANGE: fix(title, block)!: removed title BREAKING_CHANGE: use line instead Apr 1, 2024
@TadoTheMiner
Copy link
Contributor Author

This PR fixes #738

@joshka joshka changed the title fix(title, block)!: removed title BREAKING_CHANGE: use line instead fix(block)!: removed title BREAKING_CHANGE: use line instead Apr 1, 2024
@github-actions github-actions bot added the Type: Breaking Change This change will cause application code to break and must be noted in the breaking changes docs etc. label Apr 1, 2024
@joshka joshka changed the title fix(block)!: removed title BREAKING_CHANGE: use line instead fix(block)!: remove title Apr 1, 2024
@joshka joshka added this to the v0.27.0 milestone Apr 2, 2024
@TadoTheMiner TadoTheMiner marked this pull request as ready for review April 5, 2024 13:21
Copy link

codecov bot commented Apr 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.4%. Comparing base (b7778e5) to head (c62d4ec).
Report is 202 commits behind head on main.

Additional details and impacted files
@@          Coverage Diff           @@
##            main   #1012    +/-   ##
======================================
  Coverage   89.4%   89.4%            
======================================
  Files         61      60     -1     
  Lines      15430   15319   -111     
======================================
- Hits       13799   13703    -96     
+ Misses      1631    1616    -15     

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

Copy link
Member

@joshka joshka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before going further on this, I want to call your attention to the checklist in the attached issue.
It's a good idea to split this up into a few PRs:

  • add the top_ and bottom_ fields and setters, update the rendering to use these instead of the current titles field (non breaking change)
  • Implement From<Title> for Line
  • Mark all deprecations and write up a transition guide for the change in BREAKING changes
  • Remove existing deprecated setter
  • Change Block::title to accept Into instead of Into<Title>
    in a later release remove the deprecated stuff.

The plan might not be 100% accurate, but the rationale for doing things this way is that we want to provide a way to notify users of the library that things will change in the future. For this we use deprecation attributes that will make warnings on builds. This means that it's important to take a step back and do this step by step and not as one big change.

The generalized breaking change process is:

  • Make some change that will be the future state that doesn't break
  • Mark the existing code as deprecated with a note to use the new way
  • Release a couple of major versions with that new code letting users update when they can (we often make this about 2 releases, which is 2-4 months warning)
  • Finally remove the existing code (or make the breaking change)

The thing to remember is to be empathetic to the maintainers of applications and try to make their lives easy. If we break things, often we'll hear from app users instead of app maintainers (or just give a general bad impression that ratatui is buggy).

This PR as it stands is a problem as it changes things without notifying app devs. So, let's jump back to the linked issue and keep talking about the order of things instead of jumping to this implementation.

BREAKING-CHANGES.md Outdated Show resolved Hide resolved
@TadoTheMiner
Copy link
Contributor Author

Have you reviewed the transition guide?

@joshka
Copy link
Member

joshka commented Apr 11, 2024

Have you reviewed the transition guide?

This PR won't be merged as it's sort of the part 2 or 3 of a several part series and part 1 doesn't exist yet.

  1. Make a release that adds the new methods that will replace the existing ones (e.g. 0.27.0 adds title_top() and title_bottom(). Mark the deprecation in that release. This is a non-breaking change.
  2. Let that bake for a while and watch apps upgrade to the new methods when they see warnings about things breaking soon.
  3. Make a later release (0.29.0) that removes / changes the old method and removes the Title struct (i.e. this PR)

@TadoTheMiner
Copy link
Contributor Author

Have you reviewed the transition guide?

1. Make a release that adds the new methods that will replace the existing ones (e.g. 0.27.0 adds title_top() and title_bottom(). Mark the deprecation in that release. This is a non-breaking change.

Already did. So I should make a separate branch for them?

@joshka
Copy link
Member

joshka commented Apr 12, 2024

Have you reviewed the transition guide?

1. Make a release that adds the new methods that will replace the existing ones (e.g. 0.27.0 adds title_top() and title_bottom(). Mark the deprecation in that release. This is a non-breaking change.

Already did. So I should make a separate branch for them?

The first step needs to be in an entirely separate PR, merged into main, and then released into crates.io. We merge PRs as a single unit generally, not individual commits. It's possible that I've misunderstood what you've done. Can you write it out in a bit more detail about what you've done for step 1?

@TadoTheMiner
Copy link
Contributor Author

TadoTheMiner commented Apr 12, 2024

Can you write it out in a bit more detail about what you've done for step 1?

Well I didn't deprecate the old functions I've just removed. Will submit a PR that just adds the functions and deprecates the old ones.

@joshka joshka added Priority: High A bug or feature that should be out in the next release Effort: Difficult This will take a lot of effort to change / fix labels Jun 27, 2024
@joshka joshka added Status: Review Needed PR needs a review / Issue needs buy-in from other maintainers / users and removed breaking change labels Jun 27, 2024
@joshka joshka modified the milestones: v0.27.0, v0.27.1 Jun 27, 2024
@orhun
Copy link
Sponsor Member

orhun commented Sep 14, 2024

It seems like it is time to revisit this 👀

@TadoTheMiner
Copy link
Contributor Author

It seems like it is time to revisit this 👀

Yep but first We need to deprecate It #1061

@joshka
Copy link
Member

joshka commented Sep 16, 2024

I'm closing this PR out as too early to submit. This won't be something that's possible to do for another couple of releases, and in the meantime, the underlying code may change significantly enough that maintaining a long running PR would be a bad idea.

@joshka joshka closed this Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Effort: Difficult This will take a lot of effort to change / fix Priority: High A bug or feature that should be out in the next release Status: Review Needed PR needs a review / Issue needs buy-in from other maintainers / users Type: Breaking Change This change will cause application code to break and must be noted in the breaking changes docs etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants