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-27350: Add ability to require mandatory translations and also a subset #42

Merged
merged 1 commit into from Oct 30, 2020

Conversation

timj
Copy link
Member

@timj timj commented Oct 29, 2020

  • This allows for more efficient translations if only a few properties are needed.
  • This allows code to fail early if the result is required to include defined values for a property.

Copy link
Member

@RobertLuptonTheGood RobertLuptonTheGood left a comment

Choose a reason for hiding this comment

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

Just picky comments about wording of comments and error messages

If not `None`, controls the translations that will be performed
during construction. This can be useful if the caller is only
interested in a subset of the properties and knows that some of
the others might be slow to compute (which can be the case for

Choose a reason for hiding this comment

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

super-picky: "(e.g. the airmass if it has to be derived)"

TypeError
Raised if the supplied translator class was not a MetadataTranslator.
KeyError
Raised if a translation fails and pedantic mode is enabled.
Raised if a translation fails and pedantic mode is enabled or if

Choose a reason for hiding this comment

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

Does this mean,
"Raised if a required property cannot be calculated, or if pedantic mode is enabled and any translation fails"
I had to read it twice.

all_properties = set(self._PROPERTIES)
if subset is not None:
if not subset:
raise ValueError("Can not request no properties be calculated.")

Choose a reason for hiding this comment

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

"Cannot request that no properties be calculated"

* This allows for more efficient translations if only a few
  properties are needed.
* This allows code to fail early if the result is required
  to include defined values for a property.
@timj timj merged commit e39f447 into master Oct 30, 2020
@timj timj deleted the tickets/DM-27350 branch October 30, 2020 18:40
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