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

MAINT: Deduplication with abstract class #16839

Closed
wants to merge 1 commit into from

Conversation

marload
Copy link
Contributor

@marload marload commented Jul 13, 2020

config_cc and config_fc inherit abstract class config.

Copy link
Member

@mattip mattip left a comment

Choose a reason for hiding this comment

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

CI is failing: TypeError: metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases. You might want to run python setup.py build_ext locally before resubmitting.

@@ -15,7 +17,45 @@ def show_fortran_compilers(_cache=None):
dist = distutils.core._setup_distribution
show_fcompilers(dist)

class config_fc(Command):
class config(Command, metaclass=abc.ABC):
Copy link
Member

Choose a reason for hiding this comment

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

While distutils uses lower-case classes, I think we should use Upper-case. This will require more changes through the file ...

Suggested change
class config(Command, metaclass=abc.ABC):
class Config(Command, metaclass=abc.ABC):

@eric-wieser
Copy link
Member

It's probably simplest to avoid using ABC here to avoid the conflict. At any rate, those methods are hooks for the base class, so not our concern to mark abstract.

@InessaPawson
Copy link
Member

@marload Thank you for working on this! Do you think you could finish the PR some time soon?

@rossbar
Copy link
Contributor

rossbar commented Feb 25, 2022

numpy.distutils has been officially deprecated, so it's probably worth revisiting whether this change should be considered at all.

@rossbar rossbar added 57 - Close? Issues which may be closable unless discussion continued and removed 55 - Needs work labels Feb 25, 2022
@InessaPawson
Copy link
Member

@rossbar Sebastian (@seberg) reached out to me with a similar comment. I'll close the PR.

@InessaPawson InessaPawson removed the 57 - Close? Issues which may be closable unless discussion continued label Jun 24, 2022
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.

None yet

6 participants