Skip to content

bpo-15450: Allow subclassing of dircmp#5088

Closed
mitar wants to merge 1 commit into
python:masterfrom
mitar:patch-1
Closed

bpo-15450: Allow subclassing of dircmp#5088
mitar wants to merge 1 commit into
python:masterfrom
mitar:patch-1

Conversation

@mitar

@mitar mitar commented Jan 3, 2018

Copy link
Copy Markdown

Current implementation of dircmp does not really allow subclassing because it calls itself recursively.

https://bugs.python.org/issue15450

@mitar

mitar commented Jan 3, 2018

Copy link
Copy Markdown
Author

Hm, one also has to update methodmap for subclassing to work. But at least you can do this from outside.

@asvetlov

asvetlov commented Jan 3, 2018

Copy link
Copy Markdown
Contributor

I doubt if the Pull Request will be accepted without issue on bugs.python.org and unit test.

@mitar

mitar commented Jan 3, 2018

Copy link
Copy Markdown
Author

I have found this issue: https://bugs.python.org/issue15450

@asvetlov asvetlov changed the title Allow subclassing of dircmp bpo-15450: Allow subclassing of dircmp Jan 3, 2018
@asvetlov

asvetlov commented Jan 3, 2018

Copy link
Copy Markdown
Contributor

Unittests are required as well

@mitar

mitar commented Jan 3, 2018

Copy link
Copy Markdown
Author

Even if change is completely semantically the same?

@asvetlov

asvetlov commented Jan 3, 2018

Copy link
Copy Markdown
Contributor

You are adding a new behavior.
The behavior should be explicitly tested.
Otherwise nobody can guarantee that inheritance works, isn't it?

@mitar

mitar commented Jan 3, 2018

Copy link
Copy Markdown
Author

I mean, this just allows one to fix it to make it work. But if we really want to make this work properly, we should probably get rid of the whole methodmap construct and get normal methods there which call internal phase methods. Then it is easy to subclass. Currently this PR just allows one to subclass at all, but I would not recommend that the current chaos of subclassing is something officially supported.

@asvetlov

asvetlov commented Jan 3, 2018

Copy link
Copy Markdown
Contributor

Thus PR is not full. Please go forward and implement the requested feature.

@mitar

mitar commented Jan 3, 2018

Copy link
Copy Markdown
Author

Can I refactor the class to not use methodmap? Would that be something accepted?

@asvetlov

asvetlov commented Jan 3, 2018

Copy link
Copy Markdown
Contributor

Don't know, I not involved into the problem.
Maybe @cjerdonek is better person for review.
But code changes without new/updated tests have almost zero chance for merging, sure.

@cjerdonek

Copy link
Copy Markdown
Member

Thanks, @asvetlov. The patch I posted long ago on the tracker is probably a better start. I included not just the change, but also a failing test and documentation changes.

@NickCrews

Copy link
Copy Markdown
Contributor

@cjerdonek I ported over your old patch and submitted a PR at #23424 if you want to take a look. Thanks for your work!

@asvetlov

Copy link
Copy Markdown
Contributor

Closing, #23424 should land.

@asvetlov asvetlov closed this Nov 22, 2020
@mitar

mitar commented Nov 23, 2020

Copy link
Copy Markdown
Author

@NickCrews Thanks.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants