sigmf / sigmf-python

Easily interact with Signal Metadata Format (SigMF) recordings.
https://sigmf.org
GNU Lesser General Public License v3.0
44 stars 17 forks source link

validation of VERSION_KEY (core:version is currently unconditionally set to 1.0.0) #54

Closed anarkiwi closed 6 months ago

anarkiwi commented 6 months ago

Thanks so much for SigMF python, and the included validator!

One of my tools was missing core:version, but sigmf_validator found no errors. I found that is because version is unconditionally overwritten to 1.0.0 no matter what (see https://github.com/sigmf/sigmf-python/blob/c5d194d5e659def926d25737baa7b6cbbb4887bd/sigmf/sigmffile.py#L186 among others).

According to https://github.com/sigmf/SigMF/blob/sigmf-v1.x/sigmf-spec.md#sigmf-collection-format, core:version is required.

I can raise a PR to remove the explicit set so that validation fails where the version is not present/and or is set to an unexpected value. Is that the desired behavior?

Teque5 commented 6 months ago

I believe the validation should fail if the version key is absent.

With respect to your question about what version is considered valid the Specification is currently labeled 1.0.0, however I have an open issue https://github.com/sigmf/SigMF/issues/297 that tries to resolve some inconsistencies in the spec versioning and update that value.

With respect to how this module will track what is a valid core:version, we have had a few discussions without resolution. It makes sense for the validator to be able to check multiple versions of the specification, but since the schema currently lives in this repo we only have one version to check. There is an effort in the specification repo to create/maintain the schema there (https://github.com/sigmf/SigMF/issues/275 & https://github.com/sigmf/SigMF/pull/276), in which case the SigMF spec could be a git submodule of sigmf-python and in turn we could check the schema of whatever version is tagged.

The current effort on this topic is in https://github.com/sigmf/SigMF/pull/301 where @777arc wants to generate the spec directly from the schema. If that PR is merged we will likely delete the schema from this repo and go with a submodule unless someone floats a better idea.

anarkiwi commented 6 months ago

Ah that will be very nice (separate schema definition). I put in a PR to not overwrite VERSION_KEY in the meantime.

Teque5 commented 6 months ago

Closed with PR #55.