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

Raise nicer errors when legislation is invalid #742

Merged
merged 5 commits into from
May 17, 2017
Merged

Raise nicer errors when legislation is invalid #742

merged 5 commits into from
May 17, 2017

Conversation

michelbl
Copy link

legislation_tree,
state = conv.default_state,
)
compact_legislation = tax_benefit_system.get_legislation(year)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are we testing exactly ? @michelbl do you think it is relevant ?
I don't know much this part of OpenFisca.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are testing that the xml files can load. I think it is relevant, even if a lot of other tests load them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

@@ -1,5 +1,11 @@
# Changelog

### 18.2.2 - [#742](https://github.com/openfisca/openfisca-france/pull/742)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you are not modifying the openfisca_france package, you don't need to publish a new version.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only the tests are changed. But CI will complain if I don't bump the version number ?

Copy link
Member

@fpagnoux fpagnoux May 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CI should not enforce a version bump if you haven't changed any file inside openfisca_france ;)

Copy link
Member

@fpagnoux fpagnoux May 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, on second thought, improving the error is a proper nice feature, so bumping the version number is legit.

@michelbl michelbl requested a review from fpagnoux May 15, 2017 13:41
CHANGELOG.md Outdated

* Amélioration technique
* Détails :
- Met à jour les tests sur la législation à la version `Openfisca-Core` `12.1.1`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Du point de vue de l'utilisateur, ce qui change n'est pas l'adaptation des tests, mais l'amélioration du message d'erreur en cas d'erreur dans la législation.

@michelbl michelbl requested a review from fpagnoux May 16, 2017 09:52
CHANGELOG.md Outdated
@@ -2,9 +2,9 @@

### 18.2.2 - [#742](https://github.com/openfisca/openfisca-france/pull/742)

* Amélioration technique
* Amélioration de messages d'erreur
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presque : cette première ligne est normalisée : il n'y a que 4 valeurs possibles

@michelbl michelbl requested a review from fpagnoux May 17, 2017 08:50
@fpagnoux fpagnoux merged commit da42f81 into master May 17, 2017
@fpagnoux fpagnoux deleted the validation branch May 17, 2017 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants