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

refactor: utilize super().config #2083

Merged
merged 2 commits into from
May 10, 2024

Conversation

antazoey
Copy link
Contributor

@antazoey antazoey commented May 9, 2024

What I did

simple code sharing refactor and add a missing cfg doc

How I did it

How to verify it

Checklist

  • All changes are completed
  • New test cases have been added
  • Documentation has been updated

@@ -353,7 +357,7 @@ class Ethereum(EcosystemAPI):

@property
def config(self) -> EthereumConfig:
return cast(EthereumConfig, self.config_manager.get_config(self.name))
return cast(EthereumConfig, super().config)
Copy link
Member

Choose a reason for hiding this comment

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

Won't this not work unless overrided somewhere in the class heirarchy? (it will have attribute error if not subclassed)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it seems to work:

(ape310) ➜  ape git:(chore/network-cfg-clean) ape console

In [1]: networks.ethereum.config.default_network
Out[1]: 'local'

In [2]: type(networks.ethereum.config)
Out[2]: ape_ethereum.ecosystem.EthereumConfig

In [3]: networks.polygon.config.default_network
Out[3]: 'local'

In [4]: type(networks.polygon.config)
Out[4]: ape_polygon.ecosystem.PolygonConfig

What super().config does as far as I understand is call EcoystemAPI.config which is for sure a base class to Ethereum. The implementation for EcosystemAPI.config is:

    @property
    def config(self) -> PluginConfig:
        """
        The configuration of the ecosystem. See :class:`ape.managers.config.ConfigManager`
        for more information on plugin configurations.

        Returns:
            :class:`ape.api.config.PluginConfig`
        """

        return self.config_manager.get_config(self.name)

so ultimately we are sharing code by calling up to super().


But I am curious why you say what you say here in case I don't understand because ngl, some of the class stuff i Python can be gray.

Copy link
Member

Choose a reason for hiding this comment

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

As long as EcosystemAPI.config doesn't keep calling up (which it doesn't appear to do)

Copy link
Contributor Author

@antazoey antazoey May 10, 2024

Choose a reason for hiding this comment

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

It would also have to call super().config and so on

@@ -353,7 +357,7 @@ class Ethereum(EcosystemAPI):

@property
def config(self) -> EthereumConfig:
return cast(EthereumConfig, self.config_manager.get_config(self.name))
return cast(EthereumConfig, super().config)
Copy link
Member

Choose a reason for hiding this comment

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

As long as EcosystemAPI.config doesn't keep calling up (which it doesn't appear to do)

@antazoey antazoey merged commit c7c352a into ApeWorX:main May 10, 2024
17 checks passed
@antazoey antazoey deleted the chore/network-cfg-clean branch May 10, 2024 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants