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

Improvement: disambiguate the tree command from its display? #24

Open
b-enoit-be opened this issue Apr 26, 2022 · 2 comments
Open

Improvement: disambiguate the tree command from its display? #24

b-enoit-be opened this issue Apr 26, 2022 · 2 comments
Labels
enhancement New feature or request

Comments

@b-enoit-be
Copy link
Contributor

I am using manifestoo as a Python library rather than as a command line tool.

That might be a non-common use case, but I find manifestoo way easier to use that its predecessor, acsoo in that particular case, because the usage of classes makes a clear disambiguation between the display, the usage of it as a command line tool and the business logic. So one can import those classes and use them in their own Python script.

One command that is still missing a bit this simplification is the tree command which does not have a clear distinction between its business process and its display, as the last line of it prints the tree.

Wouldn't it be a good idea to have a build_tree function containing all the logic and along with a tree_command function?
This way, one could either print the tree, as it is now but could also fetch the tree and traverse it for its own purpose.

Something like:

def build_tree(
    addons_selection: AddonsSelection,
    addons_set: AddonsSet,
    odoo_series: OdooSeries,
    fold_core_addons: bool,
) -> List[Node]:
    nodes: Dict[NodeKey, Node] = {}

    def add(addon_name: str) -> Node:
        key = Node.key(addon_name)
        if key in nodes:
            return nodes[key]
        addon = addons_set.get(addon_name)
        node = Node(addon_name, addon)
        nodes[key] = node
        if not addon:
            # not found
            return node
        for depend in addon.manifest.depends:
            if depend == "base":
                continue
            node.children.append(add(depend))
        return node

    root_nodes: List[Node] = []
    for addon_name in sorted(addons_selection):
        if addon_name == "base":
            continue
        root_nodes.append(add(addon_name))

    return root_nodes

def tree_command(
    addons_selection: AddonsSelection,
    addons_set: AddonsSet,
    odoo_series: OdooSeries,
    fold_core_addons: bool,
) -> None:
    root_nodes = build_tree(addons_selection, addons_set, odoo_series, fold_core_addons)
    for root_node in root_nodes:
        root_node.print(odoo_series, fold_core_addons)
@sbidoul sbidoul added the enhancement New feature or request label Apr 27, 2022
@sbidoul
Copy link
Member

sbidoul commented Apr 27, 2022

Hi, thanks for the feedback.

I'm fine with the proposed refactoring. PR welcome.

However, if you use it as an API, do keep in mind this page of the documentation. TL;DR: there is no public documented nor stable API :)

If you want stability guarantees, we need to work on
1/ feedback and fine tuning of the public API to real use cases
2/ refactoring to expose the public API explicitly (e.g. in manifestoo.__init__.py)
3/ making sure we have complete test coverage of the API entry points
4/ generating API documentation with sphinx

Help is most welcome in this area.

@b-enoit-be
Copy link
Contributor Author

Thanks Stéphane, this was duly noted, I pinned the version I am using at a really specific version, because I red the linked page already.

I am still a recent comer in the Python/Odoo world, so I don't feel like I am up to the task just yet. I am planning to start with simpler ones to start with, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants