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

build(deps): upgrade crossterm to 0.27 #380

Merged
merged 1 commit into from
Aug 7, 2023

Conversation

a-kenji
Copy link
Contributor

@a-kenji a-kenji commented Aug 7, 2023

Changelog

Upgrade crossterm to v0.27.

I don't think this is a breaking change, since the crossterm internals aren't exposed, is that correct?

@codecov
Copy link

codecov bot commented Aug 7, 2023

Codecov Report

Merging #380 (b905805) into main (c8ddc16) will increase coverage by 0.02%.
Report is 1 commits behind head on main.
The diff coverage is 0.00%.

❗ Current head b905805 differs from pull request most recent head e7a7ac3. Consider uploading reports for the commit e7a7ac3 to get more accurate results

@@            Coverage Diff             @@
##             main     #380      +/-   ##
==========================================
+ Coverage   84.99%   85.02%   +0.02%     
==========================================
  Files          40       40              
  Lines        8686     8683       -3     
==========================================
  Hits         7383     7383              
+ Misses       1303     1300       -3     
Files Changed Coverage Δ
src/backend/crossterm.rs 0.00% <0.00%> (ø)

@sayanarijit
Copy link
Member

IMO it is a breaking change because ratatui requires us to install crossterm separately for input handling. Some extensions will break too.

@a-kenji
Copy link
Contributor Author

a-kenji commented Aug 7, 2023

You should be able to have ratatui with 0.27 and a separate crossterm for handling input events that is on 0.26.

And there shouldn't be conflicting dependencies since ratatui is not using the event feature.

Can you show me an example of an extension that will break as a result of this change?

I am happy to mark this as a breaking change if that is the consensus.

@joshka
Copy link
Member

joshka commented Aug 7, 2023

What should we do about doc comments / readme for this? Make the change now or leave it until we release? I wonder how many people use the version number from the github readme.

@a-kenji
Copy link
Contributor Author

a-kenji commented Aug 7, 2023

Thank you, that is a good point. I will update the references as well.

What should we do about doc comments / readme for this?

Since the version number is only in a doc comment I don't think it matters in this case.

Edit:
I think we should add bumping it on release to the RELEASE.md, since we don't know to which version the next release will be bumped.

@sayanarijit
Copy link
Member

Cool... I have tested with tui-input. After a cargo update, works fine.

@joshka joshka added this pull request to the merge queue Aug 7, 2023
Merged via the queue into ratatui:main with commit 37fa6ab Aug 7, 2023
@a-kenji
Copy link
Contributor Author

a-kenji commented Aug 7, 2023

Thank you for testing it!

@a-kenji a-kenji deleted the update/crossterm branch August 7, 2023 13:33
@orhun
Copy link
Member

orhun commented Aug 7, 2023

I remember Florian creating releases for only crossterm updates but I'm not sure if it is because he wanted to make the upgrade easier for the users or not. Just my 2 cents.

@a-kenji
Copy link
Contributor Author

a-kenji commented Aug 8, 2023

I remember Florian creating releases for only crossterm updates but I'm not sure if it is because he wanted to make the upgrade easier for the users or not.

I just looked through the releases of tui-rs.
It seems to me that only the last releases were specifically dependency bumps. When he was doing a release he also bumped dependencies. But more often crossterm itself was updated and what seems to me didn't influence the release schedule itself.

@joshka joshka added this to the v0.23.0 milestone Aug 21, 2023
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.

4 participants