-
Notifications
You must be signed in to change notification settings - Fork 29.6k
No more Tuple, List, Dict #38797
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
No more Tuple, List, Dict #38797
Conversation
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
The test failed with long output, that's also why I split the PR. I suggest splitting this into smaller ones. |
cc @ydshieh this is actually useful, though - we have a PR that always triggers the CI bug! |
this is not necessary due to long output, this happens from time to time, even with
Sorry, I d don't understand this command. |
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.
Thank you, I just randomly check 2 or 3 files and LGTM.
Please add @cyyever as co-author when click the merge button 🙏 Thank @cyyever to to make the library in a better shape!
The review page is somehow hanging due to large number of files.
The Too long with no output
is known CircleCI issue, you can re-run the workflow.
If it persists, ping me and let's see what we could do
For the conflict, I guess it's easier to just reset the branch, fast-forward merge the latest main, apply the same changes and merge immediately once CI is green.
9fedad4
to
a3f2528
Compare
Hi @ydshieh, sorry, I should have been clearer. What I meant was that tests always fail on this PR with the |
Ok got it. But there is also some cases where this happens with just few outputs. But for those, rerins usually works |
this seems persist 😭 . @Rocketknight1 I can take a look to see if there is some hacky workaround, but if you are up to split the PRs, go ahead too. |
I don't think this is urgent! I can wait, but if you don't think it's worth fixing in the CI let me know and I'll make split PRs instead |
Is it due to short timeout values? The check took more than 10 minutes on my host. |
It's not something from our side but CircleCI. I can probably change the configuration there but I prefer not to. Ah, I might have some idea, let me try. |
I was able to make (I will try to see if I can make it available in a better way later) The 2 failing tests seems valid, so @Rocketknight1 could you take a look for them? When you want to commit the changes, use something like
Before merge, ping me for the revert (of some changes I made) |
@ydshieh nice, thank you! I'll get the CI to pass here and then ping you again. |
Just in case you need it, the change I made is to change from doctest_list = get_doctest_files()
print(f"\n### DOCTEST TO RUN ###\n{_print_list(doctest_list)}") to # doctest_list = get_doctest_files()
doctest_list = []
print(f"\n### DOCTEST TO RUN ###\n{_print_list(doctest_list)}") (and when push changes, use the commit message I mentioned) |
8fd8aa2
to
8a9ade4
Compare
cc @ydshieh tests are passing! Should be ready to revert + merge now |
Thanks @Rocketknight1 Thank you @cyyever again for the initiative, and apologize that we forgot to add you as a co-author in the merge commit 🙏 . |
This PR uses @cyyever's work to update types from Tuple/List/Dict to tuple/list/dict. We also add the UP006 ruff check to require this in future.