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 table_name option to ParsingContext() #61

Merged
merged 6 commits into from
Aug 23, 2021
Merged

Conversation

will-moore
Copy link
Member

See IDR/idr0079-hartmann-lateralline#5 (comment)

Usage:

    ctx = ParsingContext(
        client, image._obj, file=csv_name, allow_nan=True,
        table_name=table_name
    )
    ctx.parse()

This change used in IDR/idr0079-hartmann-lateralline@57e507f
to create bulk annotations with custom names:

Screenshot 2021-08-18 at 22 28 56

NB: ParsingContext() has several parameters that are not used: fileid, cfg, cfgid, attach, options, batch_size, loops, ms. Seems like these should be removed, although that will break code that still includes them.

Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

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

One minor question about the default value. Otherwise this change looks fairly straightforward.

As mentioned in the related PR, I did not find any downstream code that relies on the table name and would be broken by this PR. Instead all downstream code filters by namespace.

Thanks for raising the unused parameters. As it stands, the issue is that omero metadata populate will pass arguments independently of the selected context so there is a need for some redundant declaration at the moment.
In #59 (comment), I suggested that using **kwargs might be an option to be lenient about extra parameters and declare only the relevant parameters across Context implementations.

src/omero_metadata/populate.py Outdated Show resolved Hide resolved
@sbesson
Copy link
Member

sbesson commented Aug 20, 2021

Thanks. Only two questions are:

  • can we add an integration test to capture the new keyword?
  • after that, should we release as 0.9.0 as this is a new backwards compatible API?

@will-moore
Copy link
Member Author

Added tests

@sbesson sbesson merged commit db29fcc into ome:master Aug 23, 2021
@sbesson
Copy link
Member

sbesson commented Aug 23, 2021

Now released as https://libraries.io/pypi/omero-metadata/0.9.0

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