Refactor __get_builtin_constructor on hasklib.py
Hello everybody, I am Park Tsai. I want to refactor __get_builtin_constructor on hasklib.py of python 2.7 (https://github.com/python/cpython/blob/2.7/Lib/hashlib.py#L72). This is the first time that I try to refactor code of CPython on GitHub, so I am very excited. This is __get_builtin_constructor code on hasklib.py ,as follows. def __get_builtin_constructor(name): try: if name in ('SHA1', 'sha1'): import _sha return _sha.new elif name in ('MD5', 'md5'): import _md5 return _md5.new elif name in ('SHA256', 'sha256', 'SHA224', 'sha224'): import _sha256 bs = name[3:] if bs == '256': return _sha256.sha256 elif bs == '224': return _sha256.sha224 elif name in ('SHA512', 'sha512', 'SHA384', 'sha384'): import _sha512 bs = name[3:] if bs == '512': return _sha512.sha512 elif bs == '384': return _sha512.sha384 except ImportError: pass # no extension module, this hash is unsupported. raise ValueError('unsupported hash type ' + name) When I read this code, it looks messy, so I want to refactor it and make it become more clearly . Then, it will be like this def get_builtin_constructor(name): try: if name[:3] in ('SHA','sha'): if(name[3:]=='1'): import _sha return _sha.new elif (name[3:] == '224'): import _sha256 return _sha256.sha224 elif (name[3:] == '256'): import _sha256 return _sha256.sha256 elif (name[3:] == '384'): import _sha512 return _sha512.sha384 elif (name[3:] == '512'): import _sha512 return _sha512.sha512 elif name in ('MD5', 'md5'): import _md5 return _md5.new except ImportError: pass # no extension module, this hash is unsupported. raise ValueError('unsupported hash type ' + name) I will be grateful for any help you can provide. I really appreciate any feedback you can offer! Best regards, Park Tsai !!
Hello, Welcome to the mailing list Park! On Tue, Aug 7, 2018 at 12:30 PM, 蔡銘峯 <parktasi@gmail.com> wrote:
Hello everybody, I am Park Tsai. I want to refactor __get_builtin_constructor on hasklib.py of python 2.7 (https://github.com/python/cpython/blob/2.7/Lib/hashlib.py#L72). This is the first time that I try to refactor code of CPython on GitHub, so I am very excited.
This is __get_builtin_constructor code on hasklib.py ,as follows.
def __get_builtin_constructor(name): try: if name in ('SHA1', 'sha1'): import _sha return _sha.new elif name in ('MD5', 'md5'): import _md5 return _md5.new elif name in ('SHA256', 'sha256', 'SHA224', 'sha224'): import _sha256 bs = name[3:] if bs == '256': return _sha256.sha256 elif bs == '224': return _sha256.sha224 elif name in ('SHA512', 'sha512', 'SHA384', 'sha384'): import _sha512 bs = name[3:] if bs == '512': return _sha512.sha512 elif bs == '384': return _sha512.sha384 except ImportError: pass # no extension module, this hash is unsupported.
raise ValueError('unsupported hash type ' + name)
When I read this code, it looks messy, so I want to refactor it and make it become more clearly .
Then, it will be like this
def get_builtin_constructor(name): try: if name[:3] in ('SHA','sha'): if(name[3:]=='1'): import _sha return _sha.new
elif (name[3:] == '224'): import _sha256 return _sha256.sha224
elif (name[3:] == '256'): import _sha256 return _sha256.sha256
elif (name[3:] == '384'): import _sha512 return _sha512.sha384
elif (name[3:] == '512'): import _sha512 return _sha512.sha512
IMHO, this decreases the readability of the code. Also, we're doing string slicing at every conditional statement which doesn't make much sense. What do you find not interesting with the current code? What is the motivation behind this change? Best, Sanyam -- Mozilla Rep http://www.SanyamKhurana.com Github: CuriousLearner
2.7 is for bug fixes only. Unless there is a bug to be fixed, I would leave the code as is. Mariatta On Tue, Aug 7, 2018 at 8:14 AM 蔡銘峯 <parktasi@gmail.com> wrote:
Hello everybody, I am Park Tsai. I want to refactor __get_builtin_constructor on hasklib.py of python 2.7 ( https://github.com/python/cpython/blob/2.7/Lib/hashlib.py#L72). This is the first time that I try to refactor code of CPython on GitHub, so I am very excited.
This is __get_builtin_constructor code on hasklib.py ,as follows.
def __get_builtin_constructor(name): try: if name in ('SHA1', 'sha1'): import _sha return _sha.new elif name in ('MD5', 'md5'): import _md5 return _md5.new elif name in ('SHA256', 'sha256', 'SHA224', 'sha224'): import _sha256 bs = name[3:] if bs == '256': return _sha256.sha256 elif bs == '224': return _sha256.sha224 elif name in ('SHA512', 'sha512', 'SHA384', 'sha384'): import _sha512 bs = name[3:] if bs == '512': return _sha512.sha512 elif bs == '384': return _sha512.sha384 except ImportError: pass # no extension module, this hash is unsupported.
raise ValueError('unsupported hash type ' + name)
When I read this code, it looks messy, so I want to refactor it and make it become more clearly .
Then, it will be like this
def get_builtin_constructor(name): try: if name[:3] in ('SHA','sha'): if(name[3:]=='1'): import _sha return _sha.new
elif (name[3:] == '224'): import _sha256 return _sha256.sha224
elif (name[3:] == '256'): import _sha256 return _sha256.sha256
elif (name[3:] == '384'): import _sha512 return _sha512.sha384
elif (name[3:] == '512'): import _sha512 return _sha512.sha512 elif name in ('MD5', 'md5'): import _md5 return _md5.new
except ImportError: pass # no extension module, this hash is unsupported.
raise ValueError('unsupported hash type ' + name)
I will be grateful for any help you can provide. I really appreciate any feedback you can offer!
Best regards, Park Tsai !! _______________________________________________ Python-Dev mailing list Python-Dev@python.org https://mail.python.org/mailman/listinfo/python-dev Unsubscribe: https://mail.python.org/mailman/options/python-dev/mariatta.wijaya%40gmail.c...
Hi there, Good to see you on python-dev. It's always good to see people getting excited about helping with Python's development. The changes you suggest are, unless I've missed something, purely cosmetic - affecting readability only. This implies that you feel the code as it stands isn't as readable as it could be. You must admit that "it looks messy" is a matter of opinion, and alone isn't much of a justification for making changes to a project that will reach its end of life in less than eighteen months. The code in the standard library is mostly fairly well-scrutinised, and as PEP 8 reminds us, change made for purely stylistic reasons threatens to introduce new bugs. It's not obvious how much of the developer documentation you've seen, so it might be worth mentioning https://devguide.python.org/ as a good starting point for anyone wanting to contribute. regards Steve Steve Holden On Tue, Aug 7, 2018 at 8:00 AM, 蔡銘峯 <parktasi@gmail.com> wrote:
Hello everybody, I am Park Tsai. I want to refactor __get_builtin_constructor on hasklib.py of python 2.7 (https://github.com/python/cpython/blob/2.7/Lib/hashlib. py#L72). This is the first time that I try to refactor code of CPython on GitHub, so I am very excited.
This is __get_builtin_constructor code on hasklib.py ,as follows.
def __get_builtin_constructor(name): try: if name in ('SHA1', 'sha1'): import _sha return _sha.new elif name in ('MD5', 'md5'): import _md5 return _md5.new elif name in ('SHA256', 'sha256', 'SHA224', 'sha224'): import _sha256 bs = name[3:] if bs == '256': return _sha256.sha256 elif bs == '224': return _sha256.sha224 elif name in ('SHA512', 'sha512', 'SHA384', 'sha384'): import _sha512 bs = name[3:] if bs == '512': return _sha512.sha512 elif bs == '384': return _sha512.sha384 except ImportError: pass # no extension module, this hash is unsupported.
raise ValueError('unsupported hash type ' + name)
When I read this code, it looks messy, so I want to refactor it and make it become more clearly .
Then, it will be like this
def get_builtin_constructor(name): try: if name[:3] in ('SHA','sha'): if(name[3:]=='1'): import _sha return _sha.new
elif (name[3:] == '224'): import _sha256 return _sha256.sha224
elif (name[3:] == '256'): import _sha256 return _sha256.sha256
elif (name[3:] == '384'): import _sha512 return _sha512.sha384
elif (name[3:] == '512'): import _sha512 return _sha512.sha512 elif name in ('MD5', 'md5'): import _md5 return _md5.new
except ImportError: pass # no extension module, this hash is unsupported.
raise ValueError('unsupported hash type ' + name)
I will be grateful for any help you can provide. I really appreciate any feedback you can offer!
Best regards, Park Tsai !!
_______________________________________________ Python-Dev mailing list Python-Dev@python.org https://mail.python.org/mailman/listinfo/python-dev Unsubscribe: https://mail.python.org/mailman/options/python-dev/ steve%40holdenweb.com
Dear Sanyam,Mariatta and Steve, Thanks for your feedback. I will read https://devguide.python.org/ carefully. Best regards, Park Tsai 2018-08-08 2:04 GMT+08:00 Steve Holden <steve@holdenweb.com>:
Hi there,
Good to see you on python-dev. It's always good to see people getting excited about helping with Python's development.
The changes you suggest are, unless I've missed something, purely cosmetic - affecting readability only. This implies that you feel the code as it stands isn't as readable as it could be. You must admit that "it looks messy" is a matter of opinion, and alone isn't much of a justification for making changes to a project that will reach its end of life in less than eighteen months.
The code in the standard library is mostly fairly well-scrutinised, and as PEP 8 reminds us, change made for purely stylistic reasons threatens to introduce new bugs.
It's not obvious how much of the developer documentation you've seen, so it might be worth mentioning https://devguide.python.org/ as a good starting point for anyone wanting to contribute.
regards Steve
Steve Holden
On Tue, Aug 7, 2018 at 8:00 AM, 蔡銘峯 <parktasi@gmail.com> wrote:
Hello everybody, I am Park Tsai. I want to refactor __get_builtin_constructor on hasklib.py of python 2.7 (https://github.com/python/cpy thon/blob/2.7/Lib/hashlib.py#L72). This is the first time that I try to refactor code of CPython on GitHub, so I am very excited.
This is __get_builtin_constructor code on hasklib.py ,as follows.
def __get_builtin_constructor(name): try: if name in ('SHA1', 'sha1'): import _sha return _sha.new elif name in ('MD5', 'md5'): import _md5 return _md5.new elif name in ('SHA256', 'sha256', 'SHA224', 'sha224'): import _sha256 bs = name[3:] if bs == '256': return _sha256.sha256 elif bs == '224': return _sha256.sha224 elif name in ('SHA512', 'sha512', 'SHA384', 'sha384'): import _sha512 bs = name[3:] if bs == '512': return _sha512.sha512 elif bs == '384': return _sha512.sha384 except ImportError: pass # no extension module, this hash is unsupported.
raise ValueError('unsupported hash type ' + name)
When I read this code, it looks messy, so I want to refactor it and make it become more clearly .
Then, it will be like this
def get_builtin_constructor(name): try: if name[:3] in ('SHA','sha'): if(name[3:]=='1'): import _sha return _sha.new
elif (name[3:] == '224'): import _sha256 return _sha256.sha224
elif (name[3:] == '256'): import _sha256 return _sha256.sha256
elif (name[3:] == '384'): import _sha512 return _sha512.sha384
elif (name[3:] == '512'): import _sha512 return _sha512.sha512 elif name in ('MD5', 'md5'): import _md5 return _md5.new
except ImportError: pass # no extension module, this hash is unsupported.
raise ValueError('unsupported hash type ' + name)
I will be grateful for any help you can provide. I really appreciate any feedback you can offer!
Best regards, Park Tsai !!
_______________________________________________ Python-Dev mailing list Python-Dev@python.org https://mail.python.org/mailman/listinfo/python-dev Unsubscribe: https://mail.python.org/mailman/options/python-dev/steve% 40holdenweb.com
participants (5)
-
Mariatta Wijaya -
Sanyam Khurana -
Steve Holden -
Tsai Park -
蔡銘峯