Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add a common class to store JSON / Simple API response data #71
Add a common class to store JSON / Simple API response data #71
Changes from 19 commits
930bb8c
0999dfe
f9c7601
3c38cca
24c5f9c
54f4b5e
9f35e2f
c65f593
a239739
e4fcbe0
ad34303
fdc5208
b7e7b18
b600b1b
1153c79
a5302c1
fccbaed
ad56aba
78b224c
6cfee2d
50d7dec
11b2af2
489cd1c
807bf68
420d2dc
313fbee
21a5331
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I though we were not supporting abi3? I think there was a discussion either in the cibuildwheel or some other wheel related MR that abi3 didn't make sense for us.
If this was copied from existing code fine but maybe we should open a follow up issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are planning to break ABI with every Python version, then abi3 is a bit pointless. I think we can support it and just check that the interpreter version matches exactly. So it's supposed to be that
cp311-abi3
works with all Pythons <= 311 but instead we check that the interpreter is exactlycp311
. For tools like cibuildwheel that generate wheels, there's no reason to support abi3. But if someone uses a tool like maturin to generate an abi3 wheel we should be able to consume it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we have some packages that has
abi3
abi tags in Pyodide distribution.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to keep this? As long as there is a version it's fine no? And str(version) has a non negligible runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I think it is okay to remove it now.
That check was introduced at pyodide/pyodide#2752, but I think it cannot happen on wheel files (unless someone modifies the version manually).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to go faster (no need to do it, just an idea). We can probably also subclass Version to something that has a frozen
__str__
and__hash__
so it doesn't need to be re-computed at each dict comparison..There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened an issue (#73) so that we can do it as a follow-up. I also think it is would be a good issue for new contributors if there is other sprint / tutorial events.