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 PyYAML as a dependency of omero-py #228

Merged
merged 2 commits into from
Jul 2, 2020
Merged

Conversation

sbesson
Copy link
Member

@sbesson sbesson commented Jun 30, 2020

In the current construction of omero-py, there are two public APIs which require PyYAML at runtime: the CLI import --bulk functionalitity as well as the omero.util.pydict_text_io utility consumed by a few CLI plugins (render, metadata) for loading YAML files.

497f1bd makes PyYAML a mandatory dependency of omero-py
dcde6b2 also removes the optional import yaml handling and replaces usage of the deprecated yaml.load and yaml.load_all by yaml.safe_load and yaml.safe_load_all

In the current construction of omero-py, there are two public APIs which
require PyYAML at runtime: the CLI import --bulk as well as the pydict_text_io
utility consumed by a few CLI plugins (render, metadata) for loading YAML
files.
@manics
Copy link
Member

manics commented Jul 1, 2020

In case you missed it sbesson#1

@joshmoore
Copy link
Member

The changes all look good to me. Testing today was all green (and travis is happy). Merging.

@joshmoore joshmoore merged commit 70a09f0 into ome:master Jul 2, 2020
@sbesson
Copy link
Member Author

sbesson commented Jul 2, 2020

I realize I haven't discussed versioning. My inclination would be to release by incrementing the patch version i.e. 5.7.2 as the API is not modified and the runtime dependency exception has always felt as a bug to me.

@sbesson sbesson deleted the pyyaml branch July 2, 2020 12:27
@joshmoore
Copy link
Member

I was thinking about that. I don't imagine adding a dependency, now that it happens transitively, requires a minor bump. Though now that I think about it, I wonder if anything else like https://github.com/ome/conda-omero-py/blob/master/meta.yaml needs a bump, too. We probably need to codify the steps on a requirement addition/modification.

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.

3 participants