foursquare / fsqio

A monorepo that holds all of Foursquare's opensource projects
Apache License 2.0
252 stars 54 forks source link

Add scripts to modernize to Python 3 #60

Closed Eric-Arellano closed 5 years ago

Eric-Arellano commented 5 years ago

These scripts were developed to modernize https://github.com/pantsbuild/pants as it drops Python 2 and can start to use Python 3-only features.

Eric-Arellano commented 5 years ago

cc @OniOni, @mateor, @GoingTharn

I tried these on a couple sample folders in Pants repo, but haven't actually fully used them yet because Pants is still a few days away from dropping Py2. You might want to wait to merge this until I get those upstream Pants PRs up, in case I have to make any modifications to these scripts.

mateor commented 5 years ago

Thanks Eric! As long as they don't break CI, you can choose to land these any way you prefer, incrementally or otherwise.

Is this something you think should go on PyPi? We could publish whls at some point, if you think it could work

Eric-Arellano commented 5 years ago

you can choose to land these any way you prefer, incrementally or otherwise.

Great. I know update_headers.py is good to go, but haven't used remove_builtins.py at scale yet so lets wait for that one. I'll give the go-ahead when ready.

Is this something you think should go on PyPi? We could publish whls at some point, if you think it could work

No I don't think so. These scripts are far too specific to Pants and not general enough to be wheels. They are also meant to be one-time scripts that you never run again, so the expectation is that people would copy this script into their local directory, run it, then delete it, rather than to download a wheel.

OniOni commented 5 years ago

No I don't think so. These scripts are far too specific to Pants and not general enough to be wheels. They are also meant to be one-time scripts that you never run again, so the expectation is that people would copy this script into their local directory, run it, then delete it, rather than to download a wheel.

Agreed, it would also be a non trivial refactor for something that works just as well as a script.

Eric-Arellano commented 5 years ago

This is now ready to merge. Used both scripts on upstream Pants this week and made substantial changes as a result.

Eric-Arellano commented 5 years ago

Actually we can hold off on merging for now. I'm going to write a couple more scripts like to no longer use class Foo(object). Will put them all in the same PR.

Eric-Arellano commented 5 years ago

This is now ready to land.

Eric-Arellano commented 5 years ago

bump @OniOni @mateor ready to land

OniOni commented 5 years ago

Oh sweet! I'll take a look!

OniOni commented 5 years ago

Thanks @Eric-Arellano here it is: 332e7a22526263f154dd0ac3c25389251570120d

mateor commented 5 years ago

Ah - @Eric-Arellano++

I see this on master, always great to see your work!