Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatically validate version number increments #1006

Open
MattiSG opened this issue Apr 16, 2021 · 3 comments
Open

Automatically validate version number increments #1006

MattiSG opened this issue Apr 16, 2021 · 3 comments
Assignees
Labels
kind:feat A feature request, a feature deprecation

Comments

@MattiSG
Copy link
Member

MattiSG commented Apr 16, 2021

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:

openfisca/openfisca-france#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).
@bonjourmauko
Copy link
Member

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 bonjourmauko self-assigned this Apr 17, 2021
@bonjourmauko bonjourmauko added the kind:feat A feature request, a feature deprecation label Apr 17, 2021
@MattiSG
Copy link
Member Author

MattiSG commented Apr 26, 2021

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.

@bonjourmauko
Copy link
Member

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 bonjourmauko added this to the Improved releases milestone Sep 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:feat A feature request, a feature deprecation
Projects
None yet
Development

No branches or pull requests

2 participants