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

Factor outputs name conflicts #1214

Merged
merged 1 commit into from May 25, 2016

Conversation

Projects
None yet
3 participants
@dmichalowicz
Contributor

dmichalowicz commented May 19, 2016

No description provided.

else:
raise AttributeError(
'Instance of {factor} has no output called {attr!r}.'.format(
factor=type(self).__name__, attr=attribute_name,
factor=type(self).__name__, attr=name,
)
)

This comment has been minimized.

@dmichalowicz

dmichalowicz May 19, 2016

Contributor

I kept __getattr__ here to make it clearer when we should show this error message

@@ -474,6 +478,13 @@ def test_bad_output_access(self):
errmsg, "GenericCustomFactor does not have multiple outputs.",
)
# Public method name, private method name, user-defined method name.
conflicting_output_names = ['compute', '_compute', 'some_method']

This comment has been minimized.

@dmichalowicz

dmichalowicz May 19, 2016

Contributor

This is still broken for inputs as a name...

@dmichalowicz dmichalowicz changed the title from Outputs name conflicts to Factor outputs name conflicts May 19, 2016

@dmichalowicz dmichalowicz force-pushed the outputs-name-conflicts branch 2 times, most recently from a0cac8c to 120c751 May 23, 2016

@coveralls

This comment has been minimized.

coveralls commented May 24, 2016

Coverage Status

Coverage increased (+0.04%) to 81.723% when pulling 1e846b7 on outputs-name-conflicts into 96ec1fd on master.

raise InvalidOutputName(
output_name=output,
termname=type(self).__name__,
disallowed_names=disallowed_names,

This comment has been minimized.

@dmichalowicz

dmichalowicz May 24, 2016

Contributor

Should this error be showing the list of prohibited names?

This comment has been minimized.

@dmichalowicz

dmichalowicz May 24, 2016

Contributor

Note that compute is not in this list, so will still error if used as an output name. Without trying to make this too muddy, I'm thinking it should still be added explicitly to the disallowed list

@coveralls

This comment has been minimized.

coveralls commented May 24, 2016

Coverage Status

Coverage increased (+0.04%) to 81.723% when pulling 8089496 on outputs-name-conflicts into 96ec1fd on master.

@dmichalowicz dmichalowicz force-pushed the outputs-name-conflicts branch from 8089496 to 6d3b33b May 24, 2016

@dmichalowicz

This comment has been minimized.

Contributor

dmichalowicz commented May 24, 2016

@ssanderson Ready for a look

@@ -543,6 +494,72 @@ def __repr__(self):
)
def validate_dtype(termname, dtype, missing_value):
"""

This comment has been minimized.

@ssanderson

ssanderson May 24, 2016

Member

indentation here is off

else:
return super(CustomFactor, self).__getattribute__(name)
def __getattr__(self, name):

This comment has been minimized.

@ssanderson

ssanderson May 24, 2016

Member

I would probably just remove this method and make the logic in __getattribute__ look like:

if outputs is NotSpecified:
    return super(...).__getattribute__(...)
if name in outputs:
    return RecarrayField(...)
raise AttributeError(...)

This comment has been minimized.

@dmichalowicz

dmichalowicz May 25, 2016

Contributor

This won't work exactly, because if outputs is specified we would be raising an AttributeError for all other attributes. The best way I can think of to do everything in __getattribute__ is something like:

if outputs is NotSpecified:
    return super(...).__getattribute__(...)
elif name in outputs:
    return RecarrayField(...)
else:
    try:
        return super(...).__getattribute__(...)
    except AttributeError:
        raise AttributeError("No such output named...")

This comment has been minimized.

@dmichalowicz

dmichalowicz May 25, 2016

Contributor

@ssanderson Do you think this is cleaner than keeping __getattr__?

This comment has been minimized.

@ssanderson

ssanderson May 25, 2016

Member

I think it's probably slightly cleaner just because it keeps all the logic in one place. I could go either way on this though, so do what you think makes sense.

@dmichalowicz dmichalowicz force-pushed the outputs-name-conflicts branch 2 times, most recently from 9546ed4 to a54cb4f May 25, 2016

@coveralls

This comment has been minimized.

coveralls commented May 25, 2016

Coverage Status

Coverage increased (+0.04%) to 81.794% when pulling 9546ed4 on outputs-name-conflicts into d34b1b9 on master.

@coveralls

This comment has been minimized.

coveralls commented May 25, 2016

Coverage Status

Coverage increased (+0.04%) to 81.799% when pulling a54cb4f on outputs-name-conflicts into d089008 on master.

@dmichalowicz dmichalowicz force-pushed the outputs-name-conflicts branch from a54cb4f to 8648680 May 25, 2016

@dmichalowicz dmichalowicz merged commit 2559c32 into master May 25, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dmichalowicz dmichalowicz deleted the outputs-name-conflicts branch May 25, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment