Added ability to define a private _validate_card method on Header subclasses#15293
Added ability to define a private _validate_card method on Header subclasses#15293astrofrog wants to merge 1 commit into
Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
In itself, something like this makes sense. But I think we need to think a bit about the API: here you return True/False. Another option would be to let _validate raise if it wants.
But I think I like the concept of ScienceState.validate and Attribute.convert_input (latter in combination with __get__) more, where the method returns the value, possibly adjusted to match what is needed. How about doing something similar here? I could see this being useful more broadly (even, e.g., in turning a float into a string for regular cards; don't remember how it is done now...)
|
@mhvk - in the use case I have for this in compressed FITS, I want to emit a warning if a card is invalid and then proceed silently. If we followed your suggestion, would it be acceptable for |
|
@astrofrog - I think it can still up to the use case how |
|
That's for |
|
@saimn - no this is going to be used in If you would prefer, I could just do this as part of the PR to refactor |
|
I've implemented @mhvk's comments but we can continue discussing whether this is needed. An alternative would be to break existing behavior and no longer warn if a user sets/updates a keyword/value pair in a |
| f"(keyword, value, [comment]) tuple; got: {card!r}" | ||
| ) | ||
|
|
||
| if self._validate_card(card) is None: |
There was a problem hiding this comment.
I think in the new scheme this should become
card = self._validate_card(card)
if card is None:
return
Otherwise any changes made would not actually be used! (same above)
There was a problem hiding this comment.
Or is _validate_card allowed to make in-place changes to the card? That would definitely make sense too.
There was a problem hiding this comment.
I think it should be allowed to return a new card so have updated following your suggestion
| if card.comment is not None and card.comment.lower() != card.comment: | ||
| warnings.warn("Comment should be lowercase, fixing", UserWarning) | ||
| card.comment = card.comment.lower() | ||
| return card |
There was a problem hiding this comment.
Here, return card should not be indented (or there should be another clause with a return at the end).
|
I cannot really comment on the overall question, but it seems to me that perhaps that is better answered anyway after refactoring, especially if that will make |
astrofrog
force-pushed
the
fits-header-validation
branch
from
7e2f21a to
41918ee
Compare
|
@astrofrog - What is here now definitely looks good to me. On the larger question, I guess there is a general point that one preferably should not introduce new private features without also merging something that uses it. So, perhaps the next step is to make a PR with your refactor, build on top of this one (probably best reduced to a single commit)? That would allow us to see the usage in practice! |
…classes to do validation on keyword names, values, or comments
astrofrog
force-pushed
the
fits-header-validation
branch
from
41918ee to
4c39610
Compare
|
@mhvk - sounds good, I've squashed this down to a single commit and will work on opening the refactor PR. |
|
Sorry @astrofrog I'm still not convinced by this addition 😅 in parallel to the existing verification system.
Right, this is the main difference, the verification system is not called when setting a card. Currently I think there are mainly two ways to verify the cards:
All of this is not perfect and has many flaws, but I'm not sure that adding another way to validate cards will help to clarify the situation ;). And yes it's private but adding this to |
|
Ok sounds good, let's close this for now and I will focus on opening the refactor PR instead for now |
3 participants
This adds a way for header subclasses to define a private
_validate_cardmethod that can be used to e.g. emit warnings or errors depending on the card. It is intended for use inCompImageHeaderin an upcoming refactor but I thought it would be easier to propose this as a standalone PR. It is private API so does not require a changelog entry.