Skip to content

[1/N] Use list,tuple,dict for typing #38713

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

cyyever
Copy link
Contributor

@cyyever cyyever commented Jun 10, 2025

What does this PR do?

Replace typing.{List,Tuple,Dict} with {list, tuple,dict}. Due to the large amount of all changes, they are split into smaller PRs.

@cyyever cyyever force-pushed the pyup2507 branch 5 times, most recently from 7acf790 to 2e772de Compare June 10, 2025 05:43
@Rocketknight1 Rocketknight1 force-pushed the pyup2507 branch 2 times, most recently from 28b8d24 to a17e7d7 Compare June 10, 2025 12:32
@ydshieh
Copy link
Collaborator

ydshieh commented Jun 10, 2025

Happy to have these changes. But could this be produced via a single command?

@Rocketknight1
Copy link
Member

Hi @cyyever, yes, this looks good! But as @ydshieh said, for security reasons it's preferable if we can run a command to do the update ourselves, because reviewing a 15k line diff manually is challenging. Can you give us the command / script you used to make this?

@HuggingFaceDocBuilderDev

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.

@ydshieh
Copy link
Collaborator

ydshieh commented Jun 10, 2025

They are several PRs and now I am a bit lost

#37809
#37039

etc.

I'm not completely against the PRs, update ruff could be OK from time to time, there is no hard rules about the decisions.

For this PR (simply changing {List,Tuple,Dict} with {list, tuple,dict}., it's ok.

@cyyever cyyever changed the title Use list,tuple,dict for typing [1/N] Use list,tuple,dict for typing Jun 11, 2025
@cyyever
Copy link
Contributor Author

cyyever commented Jun 11, 2025

@ydshieh Thank you, these are also based on ruff and sed, focusing on these types for easier review.

@cyyever
Copy link
Contributor Author

cyyever commented Jun 11, 2025

Happy to have these changes. But could this be produced via a single command?

No because ruff is not strong enough to fix comments.
For your audit, ruff UP006 was used, followed by

sed -i -e 's/\<Dict\]/dict]/g'  src/**/*.py
sed -i -e 's/\[Dict/[dict/g'  src/**/*.py
sed -i -e 's/\<\[Dict/[dict/g'  src/**/*.py
sed -i -e 's/\<Dict\[/dict[/g'  src/**/*.py
sed -i -e 's/\<Tuple\[/tuple[/g'  src/**/*.py
sed -i -e 's/\<List\[/list[/g'  src/**/*.py
sed -i -e 's/\<typing.tuple\[/tuple[/g'  src/**/*.py
sed -i -e 's/\<typing.list\[/list[/g'  src/**/*.py

ruff format was also used after all changes.

@cyyever cyyever force-pushed the pyup2507 branch 2 times, most recently from b9d7a1c to eb23ad1 Compare June 11, 2025 01:09
@cyyever cyyever marked this pull request as draft June 11, 2025 01:15
@cyyever cyyever marked this pull request as ready for review June 11, 2025 01:35
@Rocketknight1
Copy link
Member

cc @ydshieh @cyyever, if you're okay with it, I'll make a new PR to:

  1. Add UP006 to our ruff checks
  2. Apply it
  3. Run the sed commands above

Obviously, I expect this to be exactly the same as this PR, but it's just for security reasons!

@ydshieh
Copy link
Collaborator

ydshieh commented Jun 11, 2025

Yes, go ahead. Thank you a lot.

I'm not sure what 1/N means in the title, but let's not be distracted frequently by this ruff stuffs 🙏 .

If you have a list of things to be updated before py39 reaches EOL, let's see if we can do it once (you can open an issue to describe them and we can discuss).

@cyyever
Copy link
Contributor Author

cyyever commented Jun 11, 2025

@Rocketknight1 Do what you want. But commit 23aea60 is required and should be picked to fix checkers.

@cyyever
Copy link
Contributor Author

cyyever commented Jun 12, 2025

@ydshieh I prefer following @Rocketknight1 's advice, he makes a big PR from some commands that is reproducible and be reviewed internally.

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.

4 participants