openfisca-core icon indicating copy to clipboard operation
openfisca-core copied to clipboard

Automatically validate version number increments

Open MattiSG opened this issue 4 years ago • 3 comments

Hi there!

I really enjoy OpenFisca, but I recently encountered an issue.

Here is what I did:

  • I renamed a variable and incremented the patch version number.

Here is what I expected to happen:

  • The is_version_number_acceptable script tells me that I need to increment the major version number.

Here is what actually happened:

  • All automated tests pass.

Here is data (or links to it) that can help you reproduce this issue:

https://github.com/openfisca/openfisca-france/pull/1523

Context

I identify more as a:

  • Business expert (I create tests and model legislation).
  • Developer (I create tools that use the existing OpenFisca code).

MattiSG avatar Apr 16 '21 17:04 MattiSG

Hello @MattiSG, if I understand correctly what you propose is to automatically validate semantic version number? (in the case you share for example that we can't merge if we rename a class, function, method, a signature, reduce the number of parameters, change their name… without a MAJOR ).

bonjourmauko avatar Apr 17 '21 12:04 bonjourmauko

we can't merge if we rename a class, function, method, a signature, reduce the number of parameters, change their name

Not exactly: I request an automatic validation of the API surface exposed by country packages. In that case, it would be that variable or parameter renaming, removal, variable entity assignment or type change, all require a major version bump; and that new variable or parameter introduction require a minor version bump.

MattiSG avatar Apr 26 '21 08:04 MattiSG

Yes, well this would be possible, however the risk of false-positives seems high.

  • For variable entity assignment or type change, a simple traversal of the AST before/after would do (false positive: entity rename…)

  • For new variables, a delta check after the same traversal of the AST would do: if there are variable nodes after not present before, you can assume they're new (false positive: variable rename… however this one would be OK because it would be catched after by the removal check).

  • For removed variables, the same as for new ones, a delta check but reversed (are there missing variable nodes after?)

  • For renamed variables: I have no idea how you can implement this check, but you could phony-test this by composing the new and removed variable checks: if a rename is both the addition and the removal of a variable, it will always yields for a minor and a major, and we can just assume the latter being subset of the former, an thus always requiring a major in this case.

  • For formula definitions, that's a bit trickier because they're not formalised anywhere but created dynamically at runtime. It would however be easy to formalise: assuming all formulas to be called "formula_{date in iso format}", and accepting invariably 1-3 arguments, the above checks are also implementable. For return values, however, if you return anything other than a numpy array, it should just yield an error, but the only way of catching that statically is with a type-check.

  • Finally for parameters, there's the additional step of building the syntax-tree by yourself, but then the checks are more or less the same. This would just require to make the schema of parameters more explicit, then just creating both before/after taxbenefitsystems and traversing the parameters to check for changes.

A great deal of this could be in part achieved by adapting #1044 to this spec.

bonjourmauko avatar Sep 26 '21 12:09 bonjourmauko