matthewgilbert / pdblp

pandas wrapper for Bloomberg Open API
MIT License
241 stars 67 forks source link

changed the mutable default method arguments to be None. #40

Closed finete closed 6 years ago

finete commented 6 years ago

changed the mutable default method arguments to be None. I set them to be the desired argument in the method body. the previous way can lead to undesired behavior, see this post: http://docs.python-guide.org/en/latest/writing/gotchas/

matthewgilbert commented 6 years ago

Thanks for the contribution, but could you explain what bug this is addressing? I understand the problem with having mutable types in the function defaults, such as

def append_to(element, to=None):
    if to is None:
        to = []
    to.append(element)
    return to

However, since in the code ovrds is never appended to, this should not be an issue. Since it does not suffer from the previous gotchya I prefer to set it in the function signature for stylistic reasons, i.e. it is more concise.

Keeping this context in mind could you elaborate on what the benefits of this change are?

finete commented 6 years ago

The change is for future proofing. It is somewhat of a convention to not use mutables in the func declaration unless you want to use its weird functionality.

if your wish is to be more concise might you consider type declaration ?

def bdh(self, tickers: str or list , flds: str or list , start_date: str, end_date: str, elms: list=None,
+            ovrds: list=None, longdata: bool=False)
matthewgilbert commented 6 years ago

Interesting, I was not aware of type hints. This seems like a neat feature but I think at this point not something I want to implement. I think your initial commit looks good but could you resubmit without the version change, I usually bump the version in a standalone commit and roll several feature / bug updates into version updates.

matthewgilbert commented 6 years ago

Also, I've recently added a pull request template in 1a658e153c92c1779c4eee21d72ad27b0faf2904 to try and outline some best practices for collaboration. I think the main one being opening an issue prior to submitting pull requests. Obviously these aren't relevant for this commit since these guidelines weren't around when you submitted this pull request but just for future reference.

finete commented 6 years ago

lowered back the version. in the future were would like me to add this info ? in the commit comment ?

matthewgilbert commented 6 years ago

I think in the pull request discussion, aka this thread. Everything looks good, I revised your commit (stylistic changes and squashed to one commit). This is closed in f7144db9c0134cb2f0d0f52cdfe55fa57bf657e9. Thanks!