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

DM-14770: tests mis-call PropertySet.get #52

Merged
merged 2 commits into from Jun 12, 2018
Merged

DM-14770: tests mis-call PropertySet.get #52

merged 2 commits into from Jun 12, 2018

Conversation

r-owen
Copy link
Contributor

@r-owen r-owen commented Jun 12, 2018

No description provided.

The second argument is `asArray` not a default value.
Since the test is just checking that a particular item exists,
update the test to do that correctly and idiomatically.
Copy link
Contributor

@PaulPrice PaulPrice left a comment

Choose a reason for hiding this comment

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

It's annoying we can't simply say "failDecEndCpuTime" in addMultTask, but this works.

@r-owen r-owen merged commit 598c41d into master Jun 12, 2018
@ktlim
Copy link
Contributor

ktlim commented Jun 13, 2018

We could define a __contains__ method, but I'm a little wary of having PropertySet fall into the uncanny valley of being sort of like a dict but not exactly. (And exactitude is tricky given the need for C++ usage.)

@RobertLuptonTheGood
Copy link
Member

RobertLuptonTheGood commented Jun 13, 2018 via email

@TallJimbo
Copy link
Member

My (not deeply considered) $0.02 on this is that the associative container we use to directly represent (and especially round-trip) FITS headers shouldn't try to be too dict-like, but we should probably have another class that is both usable in C++ and a true Mapping (i.e. be completely dict-like) in Python.

But I'd like to review the places we want to use these before really committing to that opinion.

@r-owen
Copy link
Contributor Author

r-owen commented Jun 13, 2018

I'm uneasy about making PropertySet and PropertyList too dict-like due to serious internal differences that make it a poor mental model. That said __contains__ seems reasonable. I think DM-13222 will help a bit by getting rid of the asArray argument to get (which can too easily be mistaken for a default argument, as per this ticket).

I wish we could use a different container for FITS header cards. PropertyList has some unfortunate limitations there (e.g. no support for missing values, no ability to COMMENT cards and such in the original order).

@ktlim ktlim deleted the tickets/DM-14770 branch August 25, 2018 06:50
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.

None yet

5 participants