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

Rename container fields to plural form #22

Merged
merged 1 commit into from
Aug 3, 2020

Conversation

jmuhlich
Copy link
Collaborator

No description provided.

@codecov-commenter
Copy link

Codecov Report

Merging #22 into master will increase coverage by 0.48%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #22      +/-   ##
==========================================
+ Coverage   95.20%   95.69%   +0.48%     
==========================================
  Files           1        1              
  Lines         396      441      +45     
==========================================
+ Hits          377      422      +45     
  Misses         19       19              
Impacted Files Coverage Δ
src/ome_autogen.py 95.69% <100.00%> (+0.48%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96c98fd...aad1413. Read the comment docs.

@tlambert03
Copy link
Owner

hmm, after looking at this, I'm less confident about not just using aliases. Seems like a fair amount of hoops to jump through since we're basically changing up the schema. I also don't know that I want to force someone to have to use our internal methods to be able to create a valid class... for instance, if someone uses ome = ome_types.OME(**random_dict) as long as that random_dict follows the naming of the OME schema, it should work. (I'm aware we've already done some other things that may have broken that ... but I'd hesitate to go farther down that road, and I'd like to go back and fix it where we have broken it)

@jmuhlich
Copy link
Collaborator Author

jmuhlich commented Jul 26, 2020 via email

@tlambert03
Copy link
Owner

also feel free to disagree and provide a counter argument :)

@jmuhlich
Copy link
Collaborator Author

Hah, my proposal to do it as a second PR was my low key way of doing that. :P

@jmuhlich
Copy link
Collaborator Author

What are your design goals for doing this as aliases? Allow both singular and plural names in constructors? What about accessing or setting an aliased attribute on an already-constructed object?

Think about what a constructor signature would look like when there is an aliased field. Will there be an arg for both names? I don't think Python's typing hints can declare args as mutually exclusive.

I think users are more likely to be familiar with JSON data and other dict-type data structures than they are with the OME schema, and those tend to use plural names for containers. For example, the Amazon Web Services JSON schemas (a widely-used schema that I have fairly frequent contact with) use plurals. For some examples see here: https://docs.aws.amazon.com/cli/latest/userguide/cli-usage-output.html#cli-usage-output-filter . I feel that as long as we document that container names are pluralized users won't be confused. We could even provide a list of such fields in our docs. If we do a good job with this thing then users should be able to just browse our docs and tab-complete in their editor or Python repl to figure out what fields are available and what values to pass, rather than starting with the OME schema and working out how to translate that to our API.

@tlambert03
Copy link
Owner

tlambert03 commented Jul 28, 2020

no definitely wouldn't want both names in the constructor. I was just envisioning adding an @property alias for each plural form (but the singular would be required for the constructor). You could also use a ClassVar to keep it out of the constructor signature... (but you'd still need a property to hook it to the alias).

But, I'm not too stuck on this, just wanted to discuss it a bit more. I buy your arguments that not many people are going to care too much about a slight divergence from the OME schema... we can stick with this PR as is if it feels right to you. If we (ever) have someone raise an issue we can reconsider at that point

@tlambert03
Copy link
Owner

I just sent you a collaborator invite. So if you're done with this, see if it lets you merge

@jmuhlich jmuhlich merged commit fa7d1fe into tlambert03:master Aug 3, 2020
@jmuhlich jmuhlich deleted the plural branch August 3, 2020 18:28
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