refgenie / refgenconf

A Python object for standardized reference genome assets.
http://refgenie.databio.org
BSD 2-Clause "Simplified" License
3 stars 6 forks source link

list function overloads python's built-in type #108

Closed nsheff closed 4 years ago

nsheff commented 4 years ago

https://github.com/refgenie/refgenconf/blob/ab9da11e748c2c0ee553aa0ec0efbe1b71437a6d/refgenconf/refgenconf.py#L237

I think this is a problem. it was at least confusing to me when I saw self.list() in a bunch of places...we shouldn't name functions that mask language built-ins...

stolarczyk commented 4 years ago

I have considered that before. Is this really a problem? This list() is defined only in RefGenConf class namespace. I think there's value in having a 1:1 name correspondence between CLI and Python API, especially because this name does not affect anything in the environment after refgenconf package import. Apparently the only affected scenario is if we wanted to use the built-in list in the class definition somewhere, like this:

class RefGenConf():
    def __init__(self):
        pass
    def list(self, genome):
        pass
    some_var = list((1,2,3)) # that's not possible
    some_var = __builtins__.list((1,2,3)) # you'd need to do this to access the built-in
    def method(self):
        var = list((1,2,3)) # this uses the built-in, as expected

I did not need to do anything like this outside of the object constructor and likely never will, that's why I didn't consider this an issue

nsheff commented 4 years ago

I think the usability issue is only one part. the other part is understandability. I did not understand self.list(...) because it's an unspoken rule that those are reserved words. I didn't even know it was possible to do that. So it's a question of code readability.

What if someone wants to write a child class that inherits from RefGenConf? This can lead to confusion.

I totally get wanting to keep with the CLI API, though. I think in this case we should just leave it, but only since the API is already published. But in this case, if starting from scratch, I think I would have not overloaded the function, I guess.