CAOM2.5 (CADC-13449) #186
CAOM2.5 (CADC-13449) #186
Conversation
There was a problem hiding this comment.
Well done!
Other general comments:
- setup.cfg specifies 120 length line width, but none of the changes make use of it.
- should the code use f-strings instead of
format? - the file encodings can be removed
- When going from 2.3 to 2.4, in the Plane class, there was some effort to keep both the energy_bands (2.4) and the em_bands (2.3) version of the metadata in the Plane class. There has been no work of that kind for the 2.4 -> 2.5 transition. Is that on purpose?
| eg: Artifact('ad:CFHT/1234567o') | ||
| where 'ad:CFHT/1234567o' is a uri that refernce the file... | ||
| eg: Artifact('cadc:CFHT/1234567o') | ||
| where 'cadc:CFHT/1234567o' is an uri that reference the file... |
There was a problem hiding this comment.
Since you're doing spelling, "references the file ..."
|
|
||
| Arguments: uri of the artifact. eg: | ||
| vos://cadc.nrc.ca!vospace/APASS/apass_north/proc/100605/n100605.fz | ||
| ad:CFHT/123456p | ||
| """ | ||
| super(Artifact, self).__init__() | ||
| self.uri = uri | ||
| try: |
There was a problem hiding this comment.
Should the method validate_uri be used here?
| for i in value: | ||
| if not isinstance(value[i], AbstractCaomEntity): | ||
| checksums.append(value[i]._id) | ||
| for i in sorted(checksums): | ||
| update_checksum(checksum, checksum[i], attribute) | ||
| updated &= update_checksum(checksum, checksum[i], attribute) |
There was a problem hiding this comment.
Should &= be |=? Either that, or a comment for why dict values need to be consistently "updated", and set and list values do not?
| for i in value: | ||
| if not isinstance(value[i], AbstractCaomEntity): | ||
| checksums.append(value[i]._id) | ||
| for i in sorted(checksums): | ||
| update_checksum(checksum, checksum[i], attribute) | ||
| updated &= update_checksum(checksum, checksum[i], attribute) | ||
| return updated | ||
| else: | ||
| raise ValueError( | ||
| 'Cannot transform in bytes: {}({})'.format(value, type(value))) | ||
|
|
||
| if b is not None: | ||
| checksum.update(b) | ||
| if logger.isEnabledFor(logging.DEBUG): |
There was a problem hiding this comment.
With the removal of the hashlib calculation, is the isEnabledFor check still needed?
| return False | ||
|
|
||
|
|
||
| def to_model_name(attribute): |
There was a problem hiding this comment.
I'd just like to confirm that id or ID is purposefully now Id, when it's not the first "word" in an attribute.
| @@ -852,7 +894,7 @@ def __init__(self, name, | |||
| self.last_executed = last_executed | |||
|
|
|||
| self._keywords = set() | |||
| self._inputs = caom_util.TypedSet(PlaneURI, ) | |||
| self._inputs = caom_util.TypedSet(str, ) | |||
There was a problem hiding this comment.
TypedSet seems like overkill for str
| self.assertEqual(binascii.hexlify( | ||
| bytearray.fromhex("e30580c1db513487f495fba09f64600e")), | ||
| binascii.hexlify(cs_uri.get_bytes()), "Round trip") | ||
| # |
There was a problem hiding this comment.
Delete the commented-out code.
| @@ -239,31 +240,17 @@ def test_complete_derived(self): | |||
| version) | |||
|
|
|||
| def test_versions(self): | |||
There was a problem hiding this comment.
This method does not have any tests for 25 against other docs. Was there a decision made to not keep this cross-version checking going forward for 25?
| except ValueError: | ||
| exception = True | ||
| self.assertTrue(exception, "Missing exception") | ||
| # |
| import logging | ||
|
|
||
| from .plane import CalibrationStatus, Ucd, Polarization | ||
|
|
||
| DATA_PKG = 'data' | ||
|
|
||
| CAOM22_SCHEMA_FILE = 'CAOM-2.2.xsd' |
There was a problem hiding this comment.
Is this definition still required? Correspondingly, there is a file caom2/caom2/data/CAOM-2.2.xsd. Does it need to be kept?
This commit includes: