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

Document Role #1187

Merged
merged 10 commits into from
Sep 26, 2023
Merged

Document Role #1187

merged 10 commits into from
Sep 26, 2023

Conversation

bonjourmauko
Copy link
Member

Technical changes

  • Document Role.

@bonjourmauko bonjourmauko requested a review from a team June 27, 2023 03:38
@bonjourmauko bonjourmauko added level:intermediate Requires average OpenFisca experience kind:docs Add or improvement of documentation labels Jun 27, 2023
@coveralls
Copy link

coveralls commented Jun 27, 2023

Coverage Status

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

Copy link
Member

@MattiSG MattiSG left a comment

Choose a reason for hiding this comment

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

Thanks @maukoquiroga!

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

@benjello
Copy link
Member

@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
Copy link
Member Author

@benjello :

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

@bonjourmauko
Copy link
Member Author

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.

Copy link
Member

@MattiSG MattiSG left a comment

Choose a reason for hiding this comment

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

At this stage, I see mostly syntax improvements, and addition of documentation. I prefer to switch to a shipping mindset and unblock this PR by applying the idea that some documentation is better than no documentation, and will approve even though I cannot guarantee the accuracy of the documentation contents themselves.

Thank you for your work and patience @maukoquiroga.

@bonjourmauko
Copy link
Member Author

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.

@bonjourmauko bonjourmauko merged commit f7d979d into master Sep 26, 2023
24 checks passed
@bonjourmauko bonjourmauko deleted the document-role branch September 26, 2023 21:16
@benjello
Copy link
Member

benjello commented Oct 2, 2023

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

@benjello
Copy link
Member

benjello commented Oct 2, 2023

See also #1194

@bonjourmauko
Copy link
Member Author

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
Copy link
Member

benjello commented Oct 3, 2023

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

@bonjourmauko
Copy link
Member Author

bonjourmauko commented Oct 3, 2023

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
Copy link
Member Author

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__.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:docs Add or improvement of documentation level:intermediate Requires average OpenFisca experience
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants