search

openfisca / openfisca-core

OpenFisca core engine. See other repositories for countries-specific code & data.
https://openfisca.org
GNU Affero General Public License v3.0
165 stars 74 forks source link

Document Role #1187

Closed bonjourmauko closed 9 months ago

bonjourmauko commented 1 year ago

Technical changes

coveralls commented 1 year ago

Coverage Status

coverage: 74.115% (+0.08%) from 74.032% when pulling 8dec6b4aea8c9b3bdd08b8e5949326ba68cb7eee on document-role into 2c04dae03206655826ee529c083d0876061b6655 on master.

benjello commented 1 year ago

@maukoquiroga could you run the openfisca-france test suite with this version of core so we stress test it in real conditions and avoid some problem down the roads.

bonjourmauko commented 9 months ago

@benjello :

2380 passed, 313 warnings in 873.19s (0:14:33)

bonjourmauko commented 9 months ago

Thanks @maukoquiroga!

I can't evaluate the relevance of the documentation itself. @benjello maybe?

Thanks @MattiSG . The documentation contains unit tests, maybe you could have an input on that.

bonjourmauko commented 9 months ago

Thanks @MattiSG :) With lots of syntax improvements and doc, my hope is it'll be easier to propose and review big changes in the future without breaking things unadvertently.

benjello commented 9 months ago

@maukoquiroga : do you think this PR may produce this bug ?

benjello commented 9 months ago

See also https://github.com/openfisca/openfisca-core/pull/1194

bonjourmauko commented 9 months ago

Hi @benjello, at first view, no (but I encountered a similar issue while working in this PR). While #1194 is a first fix, Role continues to be non-hashable because its immutability can't be assured. Thanks for the fix!

benjello commented 9 months ago

1194 fixed the trouble with the openfisca-survey-manager bug

bonjourmauko commented 9 months ago

After checking, in fact yes, sorry: this PR makes Role unhashable by giving it an __eq__. If a Role is intended to actually be hashable, then indeed __eq__ should be removed.

bonjourmauko commented 9 months ago

I'll propose a PR in 5m. I'll just add a test in the doc so we know Role is meant to be hashable. No need for the __hash__ method, just removing __eq__.