Skip to content
This repository has been archived by the owner on Dec 10, 2024. It is now read-only.

Core: OO style functions #36

Merged
merged 7 commits into from
Sep 4, 2024
Merged

Conversation

iopapamanoglou
Copy link
Contributor

Core: OO style functions

Description

Move core/util.py functions into respective classes (Node, Module, ...)
Improves ergonomics by a lot.

Checklist

Please read and execute the following:

  • My code follows the coding guidelines of this project
  • My PR title is following the contribution guidelines
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (if applicable)
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works (if applicable)
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I ran Black to format my code

Code of Conduct

By submitting this issue, you agree to follow our Code of Conduct:

Copy link
Contributor

Choose a reason for hiding this comment

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

We could trivially generate util functions that maintain the same interface as before, and would avoid breaking designs. That said, I think only Ruben falls into that bucket

@deprecated("Use instance method `nodes_with_trait` instead")
def get_all_nodes_with_trait[T](node: T, trait) -> list[Nodes]:
    return node.nodes_with_trait(trait)

etc....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah generally good idea. Had it like that. But deliberately chose to get rid of util.py for good.
This is ofc resulting in a 4.1.0 next release.

from faebryk.core.util import get_all_nodes_with_trait

esphome_components = get_all_nodes_with_trait(G, F.has_esphome_config)
esphome_components = G.nodes_with_trait(F.has_esphome_config)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the implication of using a Graph, rather than a Node that you can pre-process it with other filters etc...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, but also node should be only used if we respect the hierarchy.
Since the esphome exporter looks for connected buses this breaks hierarchy and thus is better of using the full graph.

@iopapamanoglou iopapamanoglou merged commit 207420d into main Sep 4, 2024
1 check passed
@iopapamanoglou iopapamanoglou deleted the feature/move_util_functions branch September 4, 2024 17:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants