Incorporating float.is_integer into Decimal

[ac591192-bcdd-47ee-80b2-01e59622e686]

BPO	26680
Nosy	@tim-one, @rhettinger, @tiran, @serhiy-storchaka, @rob-smallshire, @abingham
PRs	python/cpython#6121 python/cpython#22584
Files	is_integer_numeric_tower.patch: Patch to introduce is_integer to the numeric tower is_integer_decimal.patch: Patch introducing is_integer to Decimal

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields: ```python assignee = 'https://github.com/rhettinger' closed_at = created_at = labels = ['type-feature', 'library', '3.11'] title = 'Incorporating float.is_integer into Decimal' updated_at = user = 'https://github.com/rob-smallshire' ``` bugs.python.org fields: ```python activity = actor = 'rhettinger' assignee = 'rhettinger' closed = True closed_date = closer = 'rhettinger' components = ['Library (Lib)'] creation = creator = 'robert_smallshire' dependencies = [] files = ['42335', '42336'] hgrepos = [] issue_num = 26680 keywords = ['patch'] message_count = 67.0 messages = ['262702', '262704', '262714', '262715', '262721', '262724', '262726', '262824', '262846', '262848', '262850', '262852', '262882', '262900', '262901', '262904', '262906', '262909', '313551', '313579', '313645', '313654', '313655', '313674', '313680', '313681', '313683', '313689', '313690', '313693', '313707', '313870', '313872', '313873', '313874', '313878', '313880', '313881', '313882', '313895', '313897', '313901', '313902', '313904', '313907', '350164', '350172', '350173', '350190', '377774', '377921', '377924', '377925', '377926', '377971', '377972', '377996', '377999', '378005', '378060', '378127', '378138', '378198', '378199', '378227', '378295', '393148'] nosy_count = 6.0 nosy_names = ['tim.peters', 'rhettinger', 'christian.heimes', 'serhiy.storchaka', 'robert_smallshire', 'Austin Bingham'] pr_nums = ['6121', '22584'] priority = None resolution = 'remind' stage = 'resolved' status = 'closed' superseder = None type = 'enhancement' url = 'https://bugs.python.org/issue26680' versions = ['Python 3.11'] ```

[rhettinger]

Mark, why did this get merged with is_integer() still in the numeric tower? I thought we had agreed not to do that.

[mdickinson]

I interpreted "in the numeric tower" as meaning declared as an abstract method that all those registering with the class would have to implement. As you point out in msg350173, that would be problematic - adding new methods to published interfaces is pretty much a no-no for backwards compatibility reasons.

But that's not what happened here. Robert's original patch *did* add is_integer as an abstract method. The PR that was merged did not - it was added as a concrete method, as a convenience for those using the ABCs via direct subclassing rather than registering.

[rhettinger]

it was added as a concrete method,

That still amounts to making it a requirement for all numeric classes, even ones that predate this patch. So existing classes that are registered will no longer comply with the ABC.

In general, it's hard to add methods to ABCs because an ABC makes a promise about what methods are available in any class registered to that ABC.

[mdickinson]

That still amounts to making it a requirement for all numeric classes

How so? I don't follow the reasoning here.

[rhettinger]

How so?

When a concrete class registers with an ABC, it is making a promise that it implements everything in the ABC so that client code can count on all the methods being present. That is the raison d'etre for abstract base classes.

In general, we restrict ABCs to a minimal useful subset of the capabilities of concrete classes. That makes the ABC more broadly applicable and it makes life easier for implementers of concrete classes. That's why collections.abc.Set doesn't include most of the named methods present in the concrete set/frozenset types.

Likewise, we don't want to add methods to already published ABCs because existing user classes registered to the ABC stop being compliant. If an additional method is deemed essential, the technically correct way to do is to make a new ABC that subclasses from the old. For example, that's why Python 2 had to have both UserDict and IterableUserDict when we needed to add a __iter__() method.

[gvanrossum]

Possibly at some point in the future we can switch to using typing.Protocol, which requires neither registration nor explicit subclassing (it's like Hashable). This makes it easy for user code to define custom protocols as needed. So if you need something that has .keys() and .items() but you don't care about .values() you can just write

class KeysAndItems(typing.Protocol):
    def keys(self): ...
    def items(self): ...

def my_func(x: KeysAndItems):
    ...

This can be statically checked using e.g. mypy.

[mdickinson]

When a concrete class registers with an ABC, it is making a promise that it implements everything in the ABC.

Ah, interesting. I would have assumed that it's only making a promise that it registers all the methods and properties marked *abstract* in the ABC. Do you have references to back up the stronger statement?

[mdickinson]

Ok, so we need to either (a) revert PR 6121 and then re-do the changes, without the changes to numbers.py, or (b) make a second PR to undo the numbers.py changes. I think there's (calendar) time available for option (b), but I'm not going to have bandwidth to do it any time soon, so there's a risk that the current changes make it into Python 3.10 purely through inertia. What's the best approach here?

[mdickinson]

[Guido]

Possibly at some point in the future we can switch to using typing.Protocol [...]

Yes, I think there's an interesting wider problem here, that typing.Protocol might be the answer to.

For numbers and collections.abc in the std. lib., I'm happy to accept that simply having a method present in the relevant ABC class implicitly makes it a part of the interface (though it would be good if that were clarified somewhere).

But in our own code, we've found it convenient to have abc.ABC subclasses that are a combination of interface and convenience base class, with the external interface separated from the convenience methods via the @abstractmethod and @abstractproperty decorators: classes that register with the given ABC subclass must provide all methods marked with those decorators, but the other methods are not considered part of the formal interface, and not used by client code that expects only an object implementing that interface.

The alternative to that convenience is to have two classes: a separate purely abstract interface, and a base class implementing that interface that's designed for subclassing. That works too, but it's a bit unwieldy, and it's useful to have the all-in-one option available.

[rhettinger]

I would have assumed that it's only making a promise that it registers all the methods and properties marked *abstract* in the ABC. Do you have references to back up the stronger statement?

The isn't some weird or incidental promise. It is the fundamental reason that abstract base classes exist at all (and not just in Python).

From PEP-3119: "Each test carries with it a set of promises: it contains a promise about the general behavior of the class, and a promise as to what other class methods will be available."

From PEP-3119: "In addition, the ABCs define a minimal set of methods that establish the characteristic behavior of the type. Code that discriminates objects based on their ABC type can trust that those methods will always be present."

we need to either (a) revert PR 6121 and then re-do the changes, without the changes to numbers.py, or (b) make a second PR to undo the numbers.py changes.

Given that (b) can't be done in the short-run, I recommend option (a) so that there is a single clean patch and to make sure this doesn't leak into a release. The PR changed 19 files, so the longer we wait the harder it will be to revert cleanly.

[ac591192-bcdd-47ee-80b2-01e59622e686]

First, I would like to apologise for the confusion I have inadvertently caused. I didn't see Raymond's question and Guido's clear response here about not modifying the numeric tower, as it came _long_ after activity had moved to GitHub, and code had been reviewed over there. Now Raymond has linked back to here and I've caught up with the situation, I offer to make a compensating PR with reverts the changes to the numeric tower. I should be able to do this in the next day or two.

Please let me know whether you would like me to proceed, or whether you intend to handle the a partial or complete rollback among yourselves.

[rhettinger]

No worries Robert. It's all part of the process :-)

If it's okay with you. I would like to revert the incorrectly applied PR and then you can build a fresh, clean patch. Does that work for you?

[rhettinger]

New changeset 4e0ce820586e93cfcefb16c2a3df8eaefc54cbca by Raymond Hettinger in branch 'master': Revert "bpo-26680: Incorporate is_integer in all built-in and standard library numeric types (GH-6121)" (GH-22584) https://github.com/python/cpython/commit/4e0ce820586e93cfcefb16c2a3df8eaefc54cbca

[rhettinger]

Applied the reversion and removed the "release blocker" designation.

When a new PR ready, we'll be more expeditious in getting reviewed.

[ac591192-bcdd-47ee-80b2-01e59622e686]

Yes Raymond, I can prepare a new PR as soon as the faulty PR has been rolled back.

[rhettinger]

Thanks. I'm hoping that this will easy now the most of the details have already been worked out.

[rhettinger]

This has gone stale and I've been unable to contact the OP. Marking as closed for now. Please reopen if this comes back to life again and I'll review the PR.