redkyn / assigner

:black_nib: Automatically assign programming homework to students on GitLab.
MIT License
27 stars 6 forks source link

Discussion: allowing additional properties in assigner config schemas #147

Closed LinuxMercedes closed 4 years ago

LinuxMercedes commented 4 years ago

From https://github.com/redkyn/assigner/pull/146#issuecomment-656253058:

We need to bump the Assigner config version as previous versions of Assigner fail to validate a config with a "canvas-courses" key in it. I am now inclined to allow additional properties to permit semi-backwards-compatibility, but I can't recall if we had a good reason for disallowing them in the past.

The PR spurring this discussion adds a new key to Assigner's config, but does not change the semantics of the existing config entries and gracefully handles the situation where such a key is not present. If we permitted additional properties, this PR would not require a schema version bump, but as we don't, it does.

Since the issue of whether we keep additionalProperties = False in the next Assigner schema is independent of that PR, I figured it'd make sense to move the discussion to a separate issue.

LinuxMercedes commented 4 years ago

My thought is that additionalProperties = False made sense before we got versioned config schemas, as it served as a crude warning that you probably needed to update Assigner as the config semantics might have changed. Now that we have versioned schemas, and thus can explicitly state a semantics change by updating the version number, I am of two minds:

1) We can have little a additional properties, as a treat, provided:

@brhoades, tell me I'm wrong (you can't not do it)!

LinuxMercedes commented 4 years ago

Another argument for not allowing additional properties: