FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add a 'download' language for fetching pre-built binaries by fangfufu · Pull Request #3020 · pre-commit/pre-commit · GitHub

Repository navigation

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 commented Oct 6, 2023

Copy link
Copy Markdown
Contributor

This commit implements #1453 (comment)

asottile commented Oct 6, 2023 •
edited
Loading

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 commented Oct 6, 2023 •
edited
Loading

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

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

Copy link
Copy Markdown
Contributor Author

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

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)

This comment was marked as off-topic.

asottile closed this Sep 6, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL