search

Yubico / python-yubico

Python code to talk to YubiKeys
https://developers.yubico.com/python-yubico/
BSD 2-Clause "Simplified" License
229 stars 33 forks source link

Python style improvements #10

Closed ojarva closed 10 years ago

ojarva commented 10 years ago
dainnilsson commented 10 years ago

@fredrikt, has any effort been put into Python 3 compatibility for the rest of python-yubico?

@ojarva, I'm fine with fixing the file names, removal of redundant parentheses and spaces, as well as wrapping script functionality in main-functions. The code refactorings I've spotted such as for/enumerate changes also look fine.

I'm not sure about variable name changes. I don't really see any motivation for changing these, other than "Cfg" and "YK" being non-standard (in which case I think "yk", or "yubikey" would have been better alternatives for the latter). These are short-lived variables and it's clear in the code what they represent. These renamings feel more like renaming for the sake of renaming to me. I'd like those changes reverted (with the exception of "YK" and "Cfg").

Overall (and most importantly), the commit bunches a few too many changes together, making it difficult to see exactly what has been changed in each file. There are changes which alter the behavior of the program, and they are not very easy to spot (in fact, I think you've introduced a few minor bugs). Before I'm comfortable merging this I'd like the changes to be broken up into multiple commits, where the main function extractions are in a commit of their own.

fredrikt commented 10 years ago

@dainnilsson I don't see the relevance of that question. If you ask me, any changes should be towards 3.x compatibility, not any other way. But hey, you are the maintainer ;)

dainnilsson commented 10 years ago

@fredrikt The reason I ask is that I'm trying to gauge how close this project is to being Python 3 compliant. If only a handful of small changes would make the code run on Python 3 then it might be worthwhile to just make those changes. If not, then I'd rather have consistent code that follows the style conventions now and have to change a few more lines if/when we make it a goal to support Python 3. Since this type of change in particular is handled automatically by migration tools, it wouldn't actually require any extra effort at all.

fredrikt commented 10 years ago

@dainnilsson good explanation of your reasoning. thanks.