Skip to content

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
@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