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

bump readchar to >=3.0.6 #6103

Merged
merged 1 commit into from
Aug 5, 2022
Merged

bump readchar to >=3.0.6 #6103

merged 1 commit into from
Aug 5, 2022

Conversation

Cube707
Copy link
Contributor

@Cube707 Cube707 commented Jul 29, 2022

readchar released a new version, that finally fixes support for arrow-keys on windows. As this featrue is usefull to this project I recomend requiring that version.

the newer versions add support for arrow-keys on windows
@Cube707 Cube707 requested a review from zanieb as a code owner July 29, 2022 10:22
@zanieb
Copy link
Contributor

zanieb commented Aug 5, 2022

Hi! Thanks for the contribution. We keep our minimum bounds for packages as low as we actually require to preserve maximum compatibility with other projects. Since we do not require the changes in the most recent version, we won't increase our required version. Installations of Prefect will get the latest version of readchar by default and upgrades can be done safely.

@zanieb zanieb closed this Aug 5, 2022
@Cube707
Copy link
Contributor Author

Cube707 commented Aug 5, 2022

Ok, that's an unexpected reply, but your call.

Just to be sure: v3.0.5 actually worsend windows suppored compared to v3.0.4, so by that logic the requiremend should be lowerd to v3.0.4

@zanieb
Copy link
Contributor

zanieb commented Aug 5, 2022

There's nothing in our minimum version requirement preventing users from receiving the latest version. However, if we bump our minimum version and another package requires an older version, we will not be compatible with it. You can see extensive discussion of this at https://iscinumpy.dev/post/bound-version-constraints/.

@Cube707
Copy link
Contributor Author

Cube707 commented Aug 5, 2022

yes I know that. I just don't get why requiring a version that has potential to break a feature of this project is acceptable, but requiring the version that interduces the fix is not.

@zanieb
Copy link
Contributor

zanieb commented Aug 5, 2022

Ah, I see what you're saying. Apologies for the misunderstanding. If the prefect cloud select-workspace command is not working on Windows, we should bump the minimum requirement. Please link to an issue or describe the feature that's broken in the future.

@zanieb zanieb reopened this Aug 5, 2022
@Cube707
Copy link
Contributor Author

Cube707 commented Aug 5, 2022

The short of it: Windows uses two different "escape" bytes, to denote special keys that are made up of two characters. readchar-v3.0.4 only registered one of these special bytes, meaning that the speciall keys (including arrow keys) would only work on some systems. With v3.0.5 a PR was merged that changed only the four arrow keys to the other byte, making them work on some windows systems, but breaking previously working systems. With v3.0.6 this is fixed and both versions are are finally detected correctly.

See my explanation in this issue: magmax/python-readchar#66
and this comment on the now reverted bad PR: magmax/python-readchar#66

And see this commit that fixed it: magmax/python-readchar@a7c0645

@Cube707
Copy link
Contributor Author

Cube707 commented Aug 5, 2022

as I see it: if windows support is not a priority, v3.0.4 is the lowest version that is required. If windows is supposed to be supported, v3.0.6 intreduces important fixes for it and should be required.

Copy link
Contributor

@zanieb zanieb left a comment

Choose a reason for hiding this comment

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

Thanks for explaining! Technically we could make this version change based on the platform but it seems simpler to just bump our requirement in the long run.

@zanieb zanieb merged commit 590b596 into PrefectHQ:main Aug 5, 2022
@Cube707
Copy link
Contributor Author

Cube707 commented Aug 5, 2022

I would also say thats over-complicating things.

Also v3.0.5 broke BACKSPACE on posix platforms. Thats not relevant for this project, but still not bad to avoid it.

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.

2 participants