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

Prioritise 64-bit downloads over 32-bit #2311

Merged
merged 5 commits into from
Feb 21, 2024

Conversation

hugovk
Copy link
Member

@hugovk hugovk commented Sep 16, 2023

@gpshead
Copy link
Member

gpshead commented Sep 16, 2023

Could the "embeddable package" ones also sort below the "installer" ones?

That way the first Windows thing people see in list is our Recommended one.

@hugovk
Copy link
Member Author

hugovk commented Sep 17, 2023

Updated to put those matching file.name.startswith('Windows embeddable package') last:

image image

Older ones (see right column) have a different filename structure, so it doesn't match them:

image

We could match something like 'embeddable' in file.name, but maybe we don't need to worry about the older ones?

@gpshead
Copy link
Member

gpshead commented Sep 21, 2023

I wonder how many people download the windows help file (whatever that is). it could be sorted lower, probably below the installers before embedded.

regardless, this PR is good. lets ship it and iterate further later.

@hugovk
Copy link
Member Author

hugovk commented Sep 29, 2023

Updated with an explicit preferred sort order for Windows files, how does this look?

image image

@gpshead
Copy link
Member

gpshead commented Sep 29, 2023

nice!

@elibroftw
Copy link

@ewdurbin Please merge this.

windows_files = []
other_files = []
for preferred in (
'Windows installer (64-bit)',
Copy link
Member

Choose a reason for hiding this comment

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

Are these strings generated by code elsewhere in the codebase or manually input by Release Managers?

If it is manual input, this feels brittle and likely to cause confusion.

Copy link
Member

Choose a reason for hiding this comment

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

They appear to be strings stored on the ReleaseFile model via the NameSlugModel mixin. I'm pretty iffy on handling this this way honestly.

Copy link
Member

Choose a reason for hiding this comment

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

@Yh1gs for RM input

Copy link
Member

Choose a reason for hiding this comment

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

Yes, they're freeform strings, set by the script adding the ReleaseFile objects: https://github.com/python/release-tools/blob/master/add-to-pydotorg.py#L90.

Copy link
Member Author

Choose a reason for hiding this comment

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

Before checking for the exact strings, I had something that checked for substrings like "(64-bit)", "(32-bit)" and "Windows embeddable package", see the old version at: dadd924 (#2311)

Would you prefer something like that? Or some other approach?

Copy link
Member

Choose a reason for hiding this comment

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

IMHO having the strings is fine, if comments were added to reference this file from the other and vice-versa

@ofek
Copy link

ofek commented Jan 8, 2024

I'm willing to help with whatever needs doing here, just let me know.

@ambv ambv merged commit f3fa1fc into python:main Feb 21, 2024
2 checks passed
@hugovk hugovk deleted the prioritise_64bit_over_32bit branch February 21, 2024 14:43
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.

List 64-bit Windows Installer Above 32-bit Windows embedded
8 participants