search

duosecurity / duo_client_python

Python library for interacting with the Duo Auth, Admin, and Accounts APIs
https://duo.com/docs/
Other
137 stars 136 forks source link

Change an f-string to .format to restore py2.7 compatibility #196

Closed gcoxmoz closed 1 year ago

gcoxmoz commented 1 year ago

42a5a1dc58cd5f5304ceb87d704c4a863011bd5d introduced an fstring which breaks the ability for this module to run on 2.7. This returns it to a portable format.

And while you can point and laugh that there's still folks using py2, the readme (updated 2w ago) still mentions 2.7, and this isn't a particularly strong break or official declaration that you're dropping 2, so this feel like a bug.

gcoxmoz commented 1 year ago

I realize this is now moot since 4.6.1 backed the breaking change out, but I'm gonna leave this open for a little bit in case it comes back on reland.

gcoxmoz commented 1 year ago

Closing out. This was a fix to the now-reverted #188.

AaronAtDuo commented 1 year ago

@gcoxmoz We do plan to bring the reverted functionality back in the near future. In order to keep the scope down, we will probably not change the f-string portion when we do so. However, since that part is not crucial to the functionality, a follow-up commit or PR to go back to a py2-supported format will definitely be acceptable.

However, long term we will at some point need to stop trying to support Python 2. The README does mention Python 2, but only as an FYI to indicate where TLS support is available. We removed Python2 from the "tested with" section and from our CI a few months back, because the GitHub runners didn't support installing Python 2 as far as we could determine.

For one example of a change that would be impossible to fix for Python 2 support, PR https://github.com/duosecurity/duo_client_python/pull/191 will definitely not work with Python 2 if we decide to land that.

I'm open to any ideas you have about how we can try to extend Python 2 support as long as possible, but I don't see any way we can do so indefinitely. So alternately, what would you want from us in terms of a definitive "we will no longer support Python 2 as of XYZ" statement?

gcoxmoz commented 1 year ago

I'm open to any ideas you have about how we can try to extend Python 2 support as long as possible, but I don't see any way we can do so indefinitely. So alternately, what would you want from us in terms of a definitive "we will no longer support Python 2 as of XYZ" statement?

I would say you've pretty much hit the nail on the head. If the only thing breaking py2 is "we did f-strings instead of format()" then that's poor portability-coding and we should avoid that.

But when you want to drop py2, do it cleanly, explicitly, and hard.

For example, 3.7 is going EOL soon, but I don't see why folks would drop support for it unless it's causing hardship on their own code.

py2 is just an extreme form of that. I have no right to demand you keep support. But when the difference between it working and not is a trivial s/fstring/format()/ in absence of a clean declared break, yeah, let's keep it working.

gcoxmoz commented 1 year ago

Handled by #202, thanks y'all.

AaronAtDuo commented 1 year ago

Glad to help. But at some point, probably in the near future, we will have to drop py2 support. I will try my best to do it cleanly, explicitly, and hard :)

AaronAtDuo commented 1 year ago

@gcoxmoz Well, we did it. We dropped py2 support in 5.0.0. Hopefully it's clean, explicit and hard enough.

gcoxmoz commented 1 year ago

Yep, totally ticks all the boxes, thank you. Amusingly, you caught me right after I went from 4.5 to 4.7 as "here ya go, the last version that has py2 support" and broke some stuff .. there's something in 4.6 that got unhappy on py2. OH WELL!

Think fondly of those of us in the past, Future Boy. :)