Skip to content

Add a 'download' language for fetching pre-built binaries - #3020

Closed
fangfufu wants to merge 1 commit into
pre-commit:mainfrom
fangfufu:main
Closed

fangfufu wants to merge 1 commit into
pre-commit:mainfrom
fangfufu:main

Conversation

@fangfufu

@fangfufu fangfufu commented Oct 6, 2023

Copy link
Copy Markdown
Contributor

This commit implements #1453 (comment)

@asottile

asottile commented Oct 6, 2023 •

Copy link
Copy Markdown
Member

please no pathlib, dataclasses, or backslashes -- really you probably don't need any classes at all. I'm also not sure why the strange sha256 -- use hexdigest which is way more standardized.

there's a lot of code here and almost zero tests so it's not in a reviewable state -- please convert your PR to draft until you're ready for a first round

@fangfufu

fangfufu commented Oct 6, 2023 •

Copy link
Copy Markdown
Contributor Author

What do you mean by

you probably don't need any classes at all

The " the strange sha256" is subresource integrity.

What do you mean by "no backslashes"?

@asottile

asottile commented Oct 6, 2023

Copy link
Copy Markdown
Member

What do you mean by

you probably don't need any classes at all

your code has a bunch of classes with nontrivial methods -- simple plain-old-data classes is preferred -- check the approaches used by most of the rest of the codebase

The " the strange sha256" is subresource integrity.

just because it is standardized doesn't mean it is good -- we aren't optimizing for http headers where characters are at a minimum so use normal hexdigest which is far more common and much easier to compute

What do you mean by "no backslashes"?

your source code contains many very very ugly backslashes

@fangfufu

fangfufu commented Oct 8, 2023

Copy link
Copy Markdown
Contributor Author

so use normal hexdigest which is far more common

@asottile, normal checksum, i.e. the output from sha256sum or md5sum are not in hex. If we use hexdigest here, then you also need something to generate those hexdigest, right?

@asottile

asottile commented Oct 8, 2023

Copy link
Copy Markdown
Member

so use normal hexdigest which is far more common

@asottile, normal checksum, i.e. the output from sha256sum or md5sum are not in hex. If we use hexdigest here, then you also need something to generate those hexdigest, right?

what do you mean yes they are

@fangfufu

fangfufu commented Oct 8, 2023

Copy link
Copy Markdown
Contributor Author

Ah never mind, for some reason I thought md5sum and sha256sum outputs string in base64 format.

@asottile

Copy link
Copy Markdown
Member

I recently build something similar that has a format I was pretty happy with -- might be able to use that format or similar here: https://github.com/asottile/setuptools-download

@fangfufu

Copy link
Copy Markdown
Contributor Author

@asottile , do you have a preferred way of handling the different OS and architecture?

@asottile

Copy link
Copy Markdown
Member

setuptools-download handles that with markers -- essentially filters on sys.platform, platform.machine() and os.name (though os.name is perhaps redundant)

@DrummyFloyd

This comment was marked as off-topic.

@asottile asottile closed this Sep 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants