Skip to content

Small improvements to Python API#411

Open
simonas-drauksas-sensmetry wants to merge 3 commits into
mainfrom
sd/python-print-root
Open

Small improvements to Python API#411
simonas-drauksas-sensmetry wants to merge 3 commits into
mainfrom
sd/python-print-root

Conversation

@simonas-drauksas-sensmetry

@simonas-drauksas-sensmetry simonas-drauksas-sensmetry commented Jun 16, 2026

Copy link
Copy Markdown
Member

This PR contains small Python API improvements that I need for integrating Sysand into Syside ReqIF functionality, namely:

  1. The ability to obtain the project root (Sysand CLI already can do this through print-root, but not in Python)
  2. The ability to obtain a list of dependencies the current project depends on (so I could check if the ReqIF library is imported)

These changes required a bit of changes on the Sysand core / CLI, namely the introduction of "source modes":

  • own, which represents the "I only want to list the included files" and was achievable through CLI as sysand sources --no-deps (still possible)
  • own_with_deps, which is what the default is sysand sources
  • deps, which is new and returns only the dependencies without included files

While the CLI only got a new flag --only-deps and thus there is no breaking change on the CLI side, the Python API lost the include_deps argument and gained no_deps and only_deps instead to better align with the CLI. Not sure if we should consider this a breaking change, or should I try to keep what was already in the Python API?

For context, this is how I would need to get only the deps with the current API:

all_srcs = sysand.sources(root, env_path=root / ".sysand")
own_srcs = sysand.sources(root, env_path=root / ".sysand", include_deps=False)
dep_srcs = [p for p in all_srcs if p not in set(own_srcs)]

While after this PR I will be able to do:

dep_srcs = sysand.sources(root, env_path=root / ".sysand", only_deps=True)

P.S.: The commits are pretty atomic and contain nice commit messages. If not a lot of changes after review are required, I'd like NOT to squash the commits on merge.

Signed-off-by: Simonas Drauksas <simonas.drauksas@sensmetry.com>
Signed-off-by: Simonas Drauksas <simonas.drauksas@sensmetry.com>
…n API

Signed-off-by: Simonas Drauksas <simonas.drauksas@sensmetry.com>
@andrius-puksta-sensmetry

Copy link
Copy Markdown
Collaborator

Not sure if we should consider this a breaking change, or should I try to keep what was already in the Python API?

For now I don't consider bindings/core API a part of stability guarantee; too few users to matter.

@andrius-puksta-sensmetry

Copy link
Copy Markdown
Collaborator

gained no_deps and only_deps instead to better align with the CLI

It would be better to use an enum in Python API, otherwise we have to check if both no_deps and only_deps are given. IMO CLI should also change, but since this is breaking, CLI change won't happen until 0.2.0.

Comment on lines +39 to +45
class SourcesMode(Enum):
OWN = auto()
"""Only the project's own sources"""
OWN_WITH_DEPS = auto()
"""The project's own sources together with its dependencies' sources"""
DEPS = auto()
"""Only the dependencies' sources"""

@andrius-puksta-sensmetry andrius-puksta-sensmetry Jun 17, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

IMO a better design would be to have:

  • a flag/arg no_own to exclude self sources, which are included by default
  • a flag/arg include with variants:
    • deps (show deps excluding std)
    • deps-std (show deps including std)
    • std (show only std)
    • none (don't show deps)

Any combination of no_own and include is valid.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

(with no dependencies shown by default)

Comment thread bindings/py/src/lib.rs
Comment thread core/src/commands/root.rs
Comment on lines +11 to +13
let root = e.root_path();
root.canonicalize_utf8()
.map_err(|err| Box::new(FsIoError::Canonicalize(root.to_owned(), err)))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let root = e.root_path();
root.canonicalize_utf8()
.map_err(|err| Box::new(FsIoError::Canonicalize(root.to_owned(), err)))
wrapfs::canonicalize(e.root_path())

@andrius-puksta-sensmetry andrius-puksta-sensmetry left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Implementation is fine, but I really don't like the design and defaults. I'd like to make invalid states unrepresentable, my previous comment details what that would look like.

Bindings can be more or less freely altered (very few users), so I would like bindings to have my proposed design, while CLI will wait until 0.2.0 to match. Alternatively, we could release this now, but I would still like to make the change on the next breaking release, and breaking the functionality soon after releasing it is not appealing.

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.

2 participants