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

Open
wants to merge 7 commits into
base: main
Choose a base branch
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
@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).

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants