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-20073: Add makeObservationInfo #23

Merged
merged 7 commits into from Jun 13, 2019
Merged

DM-20073: Add makeObservationInfo #23

merged 7 commits into from Jun 13, 2019

Conversation

timj
Copy link
Member

@timj timj commented Jun 11, 2019

In order to get validation working I've had to also add an extra element to the PROPERTIES definitions to allow for the supplied values to be vetted to ensure they are the correct class.

@Cadair does this affect you?

timj added 5 commits June 11, 2019 14:28
Add an extra element to each entry defining a property.
The extra element is the actual Python type expected for the property.
This allows for the translated properties to be checked to ensure
they have the right type.

I considered trying to derive the class name from the current
class string (or using the type, derive the fully qualified
name) but it was easier to have the string form and the
python type explicitly at the risk of some duplication and
downstream breakage for people that define their own
properties.

The third element can be anything that can be passed to
isinstance().
This is not generally a useful thing to do because there is
no facility yet to assign properties after construction.
This lets someone create an ObservationInfo directly from
keyword/value pairs. The values are validated.
makeObservationInfo seems like a better name to me than
make_obs_info given that it really is a constructor for
ObservationInfo.
Copy link

@isullivan isullivan left a comment

Choose a reason for hiding this comment

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

A couple of very minor comments. The changes work for my test.

python/astro_metadata_translator/observationInfo.py Outdated Show resolved Hide resolved
python/astro_metadata_translator/observationInfo.py Outdated Show resolved Hide resolved
@timj timj merged commit 5c16afb into master Jun 13, 2019
@timj timj deleted the tickets/DM-20073 branch June 13, 2019 01:14
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

2 participants