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-42088 Add a check for invalid characters in measurement names #219

Merged
merged 2 commits into from Mar 15, 2024

Conversation

natelust
Copy link
Contributor

No description provided.

@natelust natelust force-pushed the tickets/DM-42088 branch 2 times, most recently from 880c9a0 to 1fcec49 Compare March 13, 2024 13:36
Copy link
Contributor

@leeskelvin leeskelvin left a comment

Choose a reason for hiding this comment

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

Looks good. One comment on naming things, and one on a raise clarification.

python/lsst/analysis/tools/atools/photometry.py Outdated Show resolved Hide resolved
Comment on lines 190 to 191
# Yes this should be a log here, as pex config give us no other
# useful way to get info into the exception.
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than a raise?

Typo: give to gives.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, because I want the pex_execution framework to raise a FieldValidationError which it will do with the check function. That exception is the one that various tools will be looking for, but there also not an easy way to raise it yourself (you need access to various objects) I thought about hacking various complicated things to make it do what you might expect, but I figured it was not worth the time. This will log an exact message, and the standard exception WILL have a message containing the actual text of the invalid value.

Avro does not like minus characters in measurement
names. Change to avoid problem.
Add a check when specifying measurement names that they do not contain
characters that will be a problem for avro schemas.
@natelust natelust merged commit c21159d into main Mar 15, 2024
8 checks passed
@natelust natelust deleted the tickets/DM-42088 branch March 15, 2024 13:12
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