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

Exclude tinycss2 hook for versions after 1.0.0 #54

Merged
merged 1 commit into from
Oct 9, 2020

Conversation

danyeaw
Copy link
Contributor

@danyeaw danyeaw commented Oct 4, 2020

tinycss2 Kozea/tinycss2#27 updated the packaging which removes the need for a hook.

@danyeaw danyeaw requested review from a team and bwoodsend and removed request for a team October 4, 2020 01:19
@bwoodsend
Copy link
Member

I take it earlier versions that did need the hook are old enough to be considered obsolete? i.e There's no real need to wrap the old hook in a if not is_module_satisfies("tinycss>=x.y.z"):?

@danyeaw
Copy link
Contributor Author

danyeaw commented Oct 5, 2020

@bwoodsend Thanks, I think I was going with the thought that the new version is out and projects should update. Keeping the hook around is more conservative, but also increases the burden for this project. Any thoughts on how long to keep the hook around in general after an update?

@danyeaw danyeaw changed the title Remove tinycss2 hook since no longer needed Exclude tinycss2 hook for versions after 1.0.0 Oct 6, 2020
@danyeaw danyeaw force-pushed the remove-tinycss2 branch 2 times, most recently from cd5d5eb to d63a291 Compare October 6, 2020 00:52
@danyeaw
Copy link
Contributor Author

danyeaw commented Oct 6, 2020

@bwoodsend I updated this PR to only exclude the hook for versions after 1.0.0. 👍

@bwoodsend
Copy link
Member

Hmm, well quite often not upgrading packages is the only way to get PyInstaller to behave itself. The latest matplotlib for example brought in changes which broke PyInstaller's hook for it. So I'd rather not jettison support just because you can upgrade your way around it in this case.

I think the status quo is just to keep the hooks until they either start getting in the way or they break something. AFAIK thinycss2 isn't causing us issues and we don't have any tests for tinycss2 so it doesn't even slow our CI. So I'd rather hang onto it for now. Maybe we could add an # XXX: This hook is redundant since [version] for now. @Legorooj Do you have any thoughts on this?

@Legorooj
Copy link
Member

Legorooj commented Oct 7, 2020

I'd prefer just a note saying # This hook isn't required after tinycss2 1.0.0, rather than changing the hook.

@danyeaw
Copy link
Contributor Author

danyeaw commented Oct 7, 2020

@Legorooj You want me to remove is_module_satisfies check and replace it with a comment?

@Legorooj
Copy link
Member

Legorooj commented Oct 8, 2020

@danyeaw yes please.

tinycss2 pyinstaller#27 updated the packaging which removes the need for a hook.
@danyeaw
Copy link
Contributor Author

danyeaw commented Oct 8, 2020

@Legorooj Ok, updates made 👍

Copy link
Member

@Legorooj Legorooj left a comment

Choose a reason for hiding this comment

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

Thanks @danyeaw

@bwoodsend bwoodsend merged commit 5907b3e into pyinstaller:master Oct 9, 2020
@danyeaw danyeaw deleted the remove-tinycss2 branch October 9, 2020 10:15
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