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

Add support for config option compose-node-name #84

Merged
merged 9 commits into from
Feb 14, 2024
Merged

Conversation

simu
Copy link
Member

@simu simu commented Jan 23, 2024

This PR adds support for the Reclass option compose-node-name.

Additionally, we implement support for compatibility flags (including helpers to set/unset/clear flags from Python), and introduce a first flag ComposeNodeNameLiteralDots. This flag configures reclass-rs to generate node metadata which matches Python reclass when compose-node-name is enabled.

The compatibility flag is required because Python reclass discards literal dots in node filepaths when compose-node-name is enabled, resulting in incorrect node metadata for nodes which have literal dots in their filename (e.g. node a.1 in the test/inventory-compose-node-name inventory).

By default, reclass-rs will preserve literal dots in the node metadata (path, and part) for node names when compose-node-name is enabled.

Checklist

  • The PR has a meaningful title. The title will be used to auto generate the changelog
  • PR contains a single logical change (to build a better changelog).
  • Update the documentation.
  • Update tests.
  • Categorize the PR by setting a good title and adding one of the labels:
    bug, enhancement, documentation, change, breaking, dependency, internal
    as they show up in the changelog
  • Link this PR to related PRs or issues.

@simu simu added the enhancement New feature or request label Jan 23, 2024

This comment was marked as outdated.

@simu simu force-pushed the feat/compose-node-name branch from c7f30d0 to 3a5f5ac Compare January 23, 2024 16:48

This comment was marked as outdated.

@simu simu force-pushed the feat/compose-node-name branch from 3a5f5ac to 784a5e0 Compare January 23, 2024 16:52

This comment was marked as outdated.

@simu simu force-pushed the feat/compose-node-name branch from 784a5e0 to 6951e43 Compare January 23, 2024 17:03

This comment was marked as outdated.

This comment was marked as outdated.

@simu simu force-pushed the feat/compose-node-name branch from e8b68b5 to 3a6d26c Compare January 23, 2024 17:09

This comment was marked as outdated.

This comment was marked as outdated.

@simu simu force-pushed the feat/compose-node-name branch from 6a32cdd to f01746d Compare January 24, 2024 08:45

This comment was marked as outdated.

@simu simu force-pushed the fix/init-class-files branch from 6d4cd9e to 0e93687 Compare January 27, 2024 16:34
@simu simu force-pushed the feat/compose-node-name branch from f01746d to e8eed26 Compare January 27, 2024 16:34

This comment was marked as outdated.

@simu simu force-pushed the feat/compose-node-name branch from 064a524 to 2eab4a3 Compare January 27, 2024 16:40

This comment was marked as outdated.

This comment was marked as outdated.

@simu simu force-pushed the feat/compose-node-name branch from 2eab4a3 to c827bac Compare January 27, 2024 18:06

This comment was marked as outdated.

@simu simu force-pushed the feat/compose-node-name branch from c827bac to 178c9f3 Compare January 27, 2024 18:09

This comment was marked as outdated.

@simu simu force-pushed the fix/init-class-files branch from 0e93687 to 7d7e934 Compare January 30, 2024 09:29
Base automatically changed from fix/init-class-files to main January 30, 2024 09:39
@simu simu force-pushed the feat/compose-node-name branch from 178c9f3 to 523cabb Compare January 30, 2024 10:15

This comment was marked as outdated.

@simu simu force-pushed the feat/compose-node-name branch from 523cabb to f41dfca Compare January 30, 2024 14:16

This comment was marked as outdated.

@simu simu marked this pull request as ready for review January 30, 2024 14:22
@simu simu requested a review from a team January 30, 2024 14:22
Copy link

@HappyTetrahedron HappyTetrahedron left a comment

Choose a reason for hiding this comment

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

I really ought to learn rust.

simu and others added 9 commits February 13, 2024 10:53
When enabled, this option allows users to define nodes in nested
directories in `nodes_path`. Node names are constructed the same way as
class names, by replacing directory separators with dots.

This commit implements support for compose-node-name in such a way that
the resulting node metadata is compatible with Python reclass.
…bled

Python reclass discards literal dots in node filepaths when
compose-node-name is enabled, resulting in incorrect node metadata for
nodes which have literal dots in their filename (e.g. node `a.1` in the
`test/inventory-compose-node-name` inventory).

This commit implements compatibility flag `ComposeNodeNameLiteralDots`.
When set, this flag tells reclass-rs to generate node metadata which
matches Python reclass when compose-node-name is enabled.

By default (i.e. without the compatibility flag), reclass-rs preserves
literal dots in node names when compose-node-name is enabled.

To implement the new default behavior, we additionally track the node's
path in `nodes_path` in `NodeInfoMeta`, so that we can ensure that
literal dots are preserved when generating the node's metadata.
PyO3's getters return a clone of the field, so we can't directly operate
on `Reclass.config.compatflags`. Instead we introduce methods
`set_compat_flag()`, `unset_compat_flag()` and `clear_compat_flags()` on
class `Reclass` to modify compatibility flags from Python.
In particular, test compat flag
The compose-node-name feature in Python Reclass treats nodes whose path
in `nodes_path` starts with a `_` as top-level nodes. We extend our
implementation of compose-node-name and the test inventory for
compose-node-name to handle such nodes the same way as Python reclass.
@simu simu force-pushed the feat/compose-node-name branch from f41dfca to 0537541 Compare February 13, 2024 09:53
Copy link

Benchmark for ef2f3fb

Click to view benchmark
Test Base PR %
Reclass::inventory() multi-threaded 1600.1±115.54µs 1640.3±138.77µs +2.51%
Reclass::inventory() single-threaded 3.6±0.03ms 3.6±0.06ms 0.00%

@simu simu merged commit 8a7e524 into main Feb 14, 2024
18 checks passed
@simu simu deleted the feat/compose-node-name branch February 14, 2024 14:54
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

Successfully merging this pull request may close these issues.

2 participants