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.

Sign up for GitHub

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

Allow composing using '+' instead of explicit compose() calls. #1

Closed
wants to merge 2 commits into from
Closed

Allow composing using '+' instead of explicit compose() calls. #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c374ac9
Add dunder add method to allow composing callables through '+' operator.
Jul 28, 2021
853fa96
Add tests for add operator and fix access of functions property.
Jul 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions normal.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,15 @@ def __call__(self, /, *args, **kwargs):
result = function(result)
return result

def __add__(self, other):
Copy link
Owner

Choose a reason for hiding this comment

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

When overriding operators, make sure to also override the reverse function (in this case __radd__).

Copy link
Owner

@mentalisttraceur mentalisttraceur Jul 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

If we did implement this as an operator overload on the compose class, we would need to make sure that acompose has correctly mirrored behavior which returned acompose instances instead of compose instances.

Mainly, because if compose has a feature, acompose should match it.

Secondarily, because acompose is a subclass of compose, so if we aren't careful, acompose will inherit this implementation from compose, and its behavior is wrong for acompose.

(Incidentally, this is a strong reminder that acompose should maybe not have been a subclass of compose. I committed the sins of using inheritance to share behavior, violating the substitutability principle, and conflating a class representing composed callables with a function which does the composition, and this is a good reminder of all of that.)

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks to the thoughts triggered by this, I went ahead and fixed acompose to not be subclass of compose.

"""Compose using '+' operator."""
if isinstance(other, compose):
return compose(*self.functions, *other.functions)
elif callable(other):
return compose(*self.functions, other)
Comment on lines +65 to +67
Copy link
Owner

@mentalisttraceur mentalisttraceur Jul 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

When a class instance constructs another instance of itself, it should use whatever class it actually is, not hardcode the class: type(self)(...) instead of compose(...).

Notice how this makes acompose work correctly too. (Debatable if acompose should be subclass of compose, and someone could maybe even argue that we should never be subclassing compose, but that's a separate issue - the point is that when writing instance methods, doing this usually helps subclasses inherit more correct behavior.)

Copy link
Owner

Choose a reason for hiding this comment

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

Now that acompose is fixed to not be a subclass of compose, everything in this comment still applies, it's just that now acompose would have to explicitly reuse the same implementation instead of implicitly inheriting it, like it now does with __init__, __repr__, and functions.

else:
raise ValueError("Can only compose callables and 'compose' instances.")
Copy link
Owner

Choose a reason for hiding this comment

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

Python operator overloads are special, so instead of raising an exception in such cases, they should return NotImplemented, because that's what tells Python to give the other operand a chance to implement the operator, if it hasn't already.

Also, if we were returning an exception here, the appropriate exception would be TypeError, not ValueError, because the distinction between callable things and not-callable things is more of a type-of-thing mismatch than a right-type-of-thing-but-wrong-in-a-more-specific-way mismatch.

Comment on lines +64 to +69
Copy link
Owner

Choose a reason for hiding this comment

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

No need to detect compose instances or *-splat .functions, since compose already does all of that. Just pass in self, other as the two argumens.

Comment on lines +64 to +69
Copy link
Owner

Choose a reason for hiding this comment

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

So putting together all the code comments, if we do implement this as an overload on this class, I would change the whole function body to just this:

Suggested change
if isinstance(other, compose):
return compose(*self.functions, *other.functions)
elif callable(other):
return compose(*self.functions, other)
else:
raise ValueError("Can only compose callables and 'compose' instances.")
if callable(other):
return type(self)(self, other)
return NotImplemented

P.S. Since compose also checks if its arguments are callable and raises a TypeError if not,

  1. if we didn't need to return NotImplemented for correct operator overload behavior, this whole function could just be return compose(self, other), and

  2. you might be asking why do if callable(other): if we could just try: return compose(self, other) and except TypeError? Because then to be really thorough, we would have to worry about possibly hiding other unrelated occurances of TypeError that type(self)(self, other) could raise (and since Python is so dynamic and overridable, almost every operation we do inside compose could in principle raise a TypeError, even if we don't ever introduce any bugs that could cause it, and even if a subclass doesn't raise any).

Copy link
Owner

@mentalisttraceur mentalisttraceur Jul 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, also, unless code is expecting that its error message will be given directly to a non-technical end-user, its error messages should be optimized for pasting into other text, not for as-is presentation to humans, so:

Suggested change
raise ValueError("Can only compose callables and 'compose' instances.")
raise ValueError("can only compose callables and 'compose' instances")

You're thinking of the error message as a stand-alone thing, which is absolutely the right mindset at the user interface boundary to a human, but at the level of utility code like compose, the error message should be thought of more as something that calling code might want to combine with other clarifying text as if it was a phrase, for example like this:

try:
    clean_up(...)
except Exception as error:
    raise CleanupError(`f"cleaning up: {error}`)

If clean_up calls compose and compose raises an error with a proper sentence message, now the error looks like this error cleaning up: Some compose error.. And of course if the above code also tried to do a proper sentence, the combined error message would be Error cleaning up: Some compose error... And obviously a colon is not a sentence break, so the next word probably shouldn't be capitalized. And imagine how much code you'd need just to wrangle error messages! Every level of the call stack would have to

  1. possibly uncapitalize the first letter if it is prefixing the error message, which cannot be done in the general case without depending on dictionaries (for any language that code you call might use in error messages), because you have to figure out what should or shouldn't be capitalized mid-sentence to not uncapitalize the wrong thing and get something like Operation aborted: cPU overheating!,
  2. getting into questions like "if the error message I got has ! at the end, should I keep that, or do I want it to become a . in this case?", and
  3. possibly remove punctuation off the end if it is suffixing the error message, which cannot be done in the general case without support for detecting single characters like the interobang and ellipsis and multi-character punctuation like !? and ... (and if code we call might produce error messages in other languages, there might be punctuation characters we don't even know exist in there).

Whereas if clean_up calls compose and compose raises an error with a message formatted as if it is a phrase in the middle of a sentence, then all those problems go away, and only the outermost GUI/presentation code needs to worry about any of that (and the worries are much reduced, because it can just slap a generic prefix like Error: or Sorry! We had a problem: and a . or whatever at the end).


@_recursive_repr_if_available
def __repr__(self):
"""Represent the composed function as an unambiguous string."""
Expand Down
40 changes: 40 additions & 0 deletions test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,46 @@ def g(s):
assert f_g.functions == (g, f) # functions exposed in execution order


def test_compose_add_compose():
def f(s):
return s + 'f'
def g(s):
return s + 'g'
f_wrapped = compose(f)
g_wrapped = compose(g)
f_g_wrapped = f_wrapped + g_wrapped

assert f_g_wrapped('') == 'gf' # Remember that it's f(g(...)) so g runs first
assert f_g_wrapped.functions == (g, f) # functions exposed in execution order


def test_compose_add_function():
def f(s):
return s + 'f'
def g(s):
return s + 'g'
f_wrapped = compose(f)
f_g_composed = f_wrapped + g
assert f_g_composed('') == 'gf' # Remember that it's f(g(...)) so g runs first
assert f_g_composed.functions == (g, f) # functions exposed in execution order
Comment on lines +14 to +35
Copy link
Owner

Choose a reason for hiding this comment

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

When we find ourselves having this much of this kind of repetition in our tests, that suggests we're testing the wrong thing, and should think more precisely about how to test just what we're adding on top of the already-tested behavior.

In this case, the + in this PR does same thing as compose itself, and we already have the prior test verifying that compose(f, g) actually composes f and g. So personally, my first reflex was that the only assertions for these two + tests should look more like

assert (compose(f) + f) == compose(f, f)

but then I remembered that we don't have == implemented for compose, and I don't want to get into designing and implementing equality check overloads for compose instances, so I guess the next best thing for the purposes of these tests is

assert (compose(f) + f).functions == compose(f, f).functions

Key point is that we want to eliminate all the irrelevant stuff. Why have both f and g? Having more than one function is primarily useful for verifying that they are ordered correctly when composed. Why execute the composed callable? Calling it is primarily useful for verifying that calling compose instances actually works. Since a + b is supposed to be equivalent to compose(a, b), and all that is already part of compose functionality, then it should be getting tested by another test (and it is) - all we need to test here is whether a + b actually produces something equivalent to compose(a, b).

Copy link
Owner

Choose a reason for hiding this comment

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

Also I think if I thought about this more, I would probably have other refactors to suggest for these tests, but I don't want to spend more time on thinking and describing how I would further refine the tests for how it is now, given my "big picture" leanings.



def test_compose_add_neither_compose_nor_function():
def f(s):
return s + 'f'
Comment on lines +39 to +40
Copy link
Owner

Choose a reason for hiding this comment

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

Since the actual function is never used, I would probably use the closest thing to an appropriately "nil" function here:

Suggested change
def f(s):
return s + 'f'
def f(_):
return None

(Of course, a completely nil function has zero arguments and no explicit return value, but function composition logically requires every function except the last one returns something, and every function except the first one takes exactly one argument, so the closest to a nil function which still fits every possible position in a function composition takes one argument and returns something.)

f_wrapped = compose(f)
g = 'string'
Copy link
Owner

Choose a reason for hiding this comment

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

This comment is just a very minor matter, almost just personal taste, but I would've used None instead of a string here, because if I'm only using one example, I like to use the most conceptually simple/basic example, and None is more simple/basic than a string (at least once you've reified nothingness into a placeholder nil value which represents the absence of a value).


raised = False

try:
f_g_wrapped = f_wrapped + g
except ValueError as err:
raised = True
finally:
assert raised
Comment on lines +44 to +51
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
raised = False
try:
f_g_wrapped = f_wrapped + g
except ValueError as err:
raised = True
finally:
assert raised
try:
f_wrapped + g
except TypeError:
return
assert False, '+ should have raised'

Because of a bunch of tiny optimizations of various factors:

Simpler (less logical operations, less indirection, less things, less complecting of things together), shorter (so more relevant information fits within the detailed central spot of human vision, so you pay less saccades and human working memory for reading the same code behavior), produces a relevant self-explanatory failure message, not naming/storing things which we're not using as if we're going to use them (so the reader has more immediate local cues that they eliminate or discount the possibility that they are used later), the error type matches what Python normally raises in these cases.

Actually, testing if an error is raised is not the right test - Python is responsible for raising that TypeError if both operands return NotImplemented. We as operator overloaders are responsible for returning NotImplemented. So on further thought I'd go with:

Suggested change
raised = False
try:
f_g_wrapped = f_wrapped + g
except ValueError as err:
raised = True
finally:
assert raised
result = f_wrapped.__add__(g)
assert result is NotImplemented



Copy link
Owner

@mentalisttraceur mentalisttraceur Jul 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Also, I think these tests only felt like they belonged in this spot of the file because the first two tests start out by re-testing everything that the first test tests. But once we clear that up, it becomes more obvious that these tests are not particularly related to the first test at all, and thus are just testing a distinct feature.

And in fact they're breaking up the flow of the tests, because the tests that immediately follow are still testing compose functionality, while these new tests are testing the layered-on addition of + behaving as an alias/shorthand for compose.

def test_inlining():
def f(_):
return None
Expand Down