Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DM-14204: Modernize python in ip_* packages #54

Merged
merged 4 commits into from Apr 30, 2018
Merged

Conversation

r-owen
Copy link
Contributor

@r-owen r-owen commented Apr 25, 2018

No description provided.

@r-owen r-owen force-pushed the tickets/DM-14204 branch 3 times, most recently from a6db196 to 7df2c48 Compare April 26, 2018 00:29
@timj
Copy link
Member

timj commented Apr 26, 2018

If future is no longer used please remove python_future from the table file.

Eliminate use of future and __future__
Copy link
Contributor

@natelust natelust left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address the one issue, everything else looks ok

def runDataRef(_self, dataRef):
return Struct(exposure=self.exposure)
def runDataRef(self, dataRef, exposure=self.exposure):
return Struct(exposure=exposure)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get here that self needs fixed. However, why did you move exposure to the arguments list? Even if this is needed in the arguments, I don't see how this syntax is correct (baring some odd variable scoping). The self variable is not bound until entering the function body, before that it is just part of the parameter pack. Therefore exposure=self.exposure should not even be able to be parsed by python. If it can be then the "self" in exposure=self.exposure must be part of some clouser or outer class level scope, and is not necessarily the "self" that is the first argument to the function. If exposure is not needed in the call signature the function behavior should go back to what it was. If it is needed the function should be something like:

def runDataRef(self, dataRef, exposure=None):
    if exposure is None:
        exposure = self.exposure
    return Struct(exposure=exposure)

Copy link
Contributor Author

@r-owen r-owen Apr 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The self in exposure=self.exposure is the self of the outer test class. This simply binds exposure, which otherwise is inaccessible because self that is the first argument of runDataRef hides it. It works, but I agree it's confusing to read. I'll try to figure out something less confusing.

I don't think we can fix the E266 violations from
`##` Doxygen blocks without breaking the Doxygen
@r-owen r-owen merged commit 82d6eed into master Apr 30, 2018
@ktlim ktlim deleted the tickets/DM-14204 branch August 25, 2018 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants