SigMF icon indicating copy to clipboard operation
SigMF copied to clipboard

rework validate using official jsonschema

Open Teque5 opened this issue 2 years ago • 6 comments

Okay, this was a tough one. Definitely closes #186, #174. I think this also closes #176.

rework validate using official jsonschema

Changes

  • Remove concept of sections throughout.
  • Removed custom json validation framework, switch to official jsonschema
  • Rewrite meta schema.
  • validate() is run on file write automatically
  • Improve validation testing.

API Change

  • SigMFFile.validate() no longer returns True or ValidationError. Will return None or raise ValidationError

This commit increments minor version to 1.1.0.

Issues

  • Unable to get defaults working in schema definition, but when that occurs get_default_metadata for SigMFFile should be created dynamically.
  • There are no tests for the collection schema.
  • The schema can be improved dramatically.

I was using jsonschema.net to help figure out the schemas, but a lot of the free software seems to be of varying quality. I'd love it if someone else gave this a once-over. This schema linter was also useful in debugging.

Teque5 avatar Jan 29 '22 23:01 Teque5

@Teque5 First of all, thank you for what looks like a pretty big lift today!

I looked at every line of the diff, but didn't understand the context for much of it. I'm commenting because I have been wondering whether a significant tools bump like this would deserve a minor level version increment or a patch level version increment (the 2nd or the 3rd number).

I see that you've moved the version to 1.1.0, and I have a suggestion for the group that I'm not 100% sold on myself, but is worth discussing.

The thought is to only perform minor-level version increments for backwards-compatible changes to the spec, and to have all tooling-related changes result in only a patch-level version increment.

This approach puts the emphasis of the version squarely on the spec, making the tools feel less important, but they will get over it. This improved validation support is significant, but it's an incremental improvement for support of the spec.

Of course, major-level version increments would then only happen when some backwards-incompatible change is made to the spec. If we want to differentiate between tools improvements and backwards-compatible spec improvements, then that probably falls to patch-level and minor-level, respectively.

Another approach would be to increment a x.y.z.w 4-number version scheme, but that's probably not the most attractive.

What do you think?

gmabey avatar Jan 30 '22 02:01 gmabey

@gmabey I really don't think we should deviate from the standard major.minor.micro versioning and be something special. Usually if there is an API change you increment minor, but since this version is more related to the spec I can change it to micro if others agree. That would make this 1.0.2 if it's the next merged PR.

Teque5 avatar Jan 30 '22 02:01 Teque5

@gmabey and if there are places in the code you are wondering about, just comment with a ? or something and I'll do a quick summary. I hadn't looked at the validator code in a while, but I just spent 12 hrs staring at it so I know how everything works right now.

Teque5 avatar Jan 30 '22 02:01 Teque5

@Teque5 thanks for getting this together. I'll take a more in depth look and do a proper review this week.

Regarding versioning, I agree there is no reason to overcomplicate this. The right way to address specification vs tooling versioning differences is to break them out into separate efforts, and we can do so if we eventually feel it necessary.

Because this changes the API it will need to wait for the next minor version most likely, though we could simply retain the True/False behavior and preserve the API for now and make the bulk of the change to 1.0.2 which is probably my preference though its not super important at the moment and I have noted "versioning" as something to discuss on the next working group call.

Can you elaborate more on why you made the API change?

jacobagilbert avatar Jan 30 '22 15:01 jacobagilbert

Can you elaborate more on why you made the API change?

The validate() functionality was originally written for v0.0.1 and was doing some un-pythonic things. Specifically it would either return True or a special error object called ValidationResult which always required special handling for functions calling validate(). As implemented now it will always return None and will throw an exception related to jsonschema if there is an issue. If desired, the user can then use try...except to catch the error if they need to. I prefer this more pythonic implementation.

Teque5 avatar Jan 30 '22 23:01 Teque5

I've rebased the PR to v1.x and if someone reviews it @gmabey we can merge. This would increment version to v1.1.0 since the API changes for validation.

Teque5 avatar Aug 14 '22 18:08 Teque5