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

Adding options to the Python plugin from_bytes() and to_bytes() methods #880

Conversation

jesper-friis
Copy link
Collaborator

@jesper-friis jesper-friis commented Jul 5, 2024

Description

Adding options to the Python plugin from_bytes() and to_bytes() methods.

Note, that this PR will not make old Python plugins with to_bytes() and/or from_bytes() methods without the options argument stop working.

Type of change

  • Bug fix & code cleanup
  • New feature
  • Documentation update
  • Test update

Checklist for the reviewer

This checklist should be used as a help for the reviewer.

  • Is the change limited to one issue?
  • Does this PR close the issue?
  • Is the code easy to read and understand?
  • Do all new feature have an accompanying new test?
  • Has the documentation been updated as necessary?

@jesper-friis jesper-friis linked an issue Jul 5, 2024 that may be closed by this pull request
bindings/python/dlite-python.i Outdated Show resolved Hide resolved
storages/python/dlite-plugins-python.c Outdated Show resolved Hide resolved
storages/python/dlite-plugins-python.c Outdated Show resolved Hide resolved
with_uuid: Whether to include UUID in the output.
options: Supported options:
- `soft7`: Whether to structure metadata as SOFT7.
- `with_uuid`: Whether to include UUID in the output.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we have two argumenter for the same things?

Copy link
Collaborator Author

@jesper-friis jesper-friis Jul 5, 2024

Choose a reason for hiding this comment

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

soft7 and with_uuid are not the same thing. With soft7=True dimensions and properties will be dicts, otherwise they will be arrays. with_uuid=True simply adds an uuid key to the return representation.
Or did I misunderstood your question?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, I measn with_uuid and single. They have the same description.

Copy link
Collaborator Author

@jesper-friis jesper-friis Jul 6, 2024

Choose a reason for hiding this comment

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

For me the option descriptions look different:

    Arguments:
        location: Path to YAML file.
        options: Supported options:
        - `mode`: Mode for opening.  Valid values are:
            - `a`: Append to existing file or create new file (default).
            - `r`: Open existing file for read-only.
            - `w`: Truncate existing file or create new file.
        - `soft7`: Whether to save using SOFT7 format.
        - `single`: Whether the input is assumed to be in single-entity form.
          If "auto" (default) the form will be inferred automatically.
        - `with_uuid`: Whether to include UUID when saving.

The with_uuid=true option includes an "uuid" field next to "meta", "description", "dimensions" and "properties", while single=false creates another level of nesting using the UUID as key for the instance. So with single=true;with_uuid=false no UUID will be included in the output. While with single=false;with_uuid=true the UUID will be shown twice.

But I see some possible improvements:

  • with_uuid could be renamed to with-uuid for consistency with the json plugin
  • it would be more useful to use the single option for controlling for output (this is how it is used in the json plugin)
  • with PR Added support for to_bytes() and from_bytes() in the json storage plugin #881, the json plugin could be used directly, instead of relying on separate Python-implementations of Instance.asdict() and dlite.utils.instance_from_dict(). That would simplify things and improve consistency

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just look at line 158 and 159 here (above and below this comment section. 'single' and 'with_uuid' have the same description.

I think it is very good to be consistent with the json plugin.

jesper-friis and others added 5 commits July 5, 2024 20:49
Co-authored-by: Francesca L. Bleken <48128015+francescalb@users.noreply.github.com>
Co-authored-by: Francesca L. Bleken <48128015+francescalb@users.noreply.github.com>
Co-authored-by: Francesca L. Bleken <48128015+francescalb@users.noreply.github.com>
…tes' of github.com:SINTEF/dlite into 868-add-options-to-instanceto_bytes-and-instancefrom_bytes
Copy link
Collaborator

@francescalb francescalb left a comment

Choose a reason for hiding this comment

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

Approved, Under the condition that the description on with_uuid and single is updated per my comment.

@jesper-friis jesper-friis merged commit c512b6b into master Jul 6, 2024
15 checks passed
@jesper-friis jesper-friis deleted the 868-add-options-to-instanceto_bytes-and-instancefrom_bytes branch July 6, 2024 20:25
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.

Add options to Instance.to_bytes() and Instance.from_bytes()
2 participants