-
Notifications
You must be signed in to change notification settings - Fork 75
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
docs: add typing to commons #1054
Conversation
2a82c1f
to
172c516
Compare
13b8464
to
27d4639
Compare
595ed67
to
b3e5289
Compare
b3e5289
to
a7a5c4c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After a video discussion with @maukoquiroga and a bit of research (see also #1033 (comment)), I understand that adding type hints:
- Will have no functional impact on OpenFisca.
- Will not force contributors to add their own type hints.
- Will not force country packages to add their own type hints.
- Will improve generated documentation.
- Will improve readability of the code base.
- Will improve testability of the code base.
- Opens up the way for future optimisations: if the whole codebase gets type-annotated, it will be easier to compile, to statically analyse, and to optimise for performance.
In order to minimise install performance side-effects and burden, I requested that additional dependencies are kept to a minimum, which has been done today.
Under these conditions, I believe this changeset adds value and does not warrant a preliminary community agreement, as it should be considered mainly as documentation improvement.
I suggest type hints to be encouraged in future PRs, but not required. Once production and contribution feedback has been obtained, we should consider opening an RFC on making type hints mandatory for future contributions along with strict CI checks.
I see that my approving review has been dismissed. Please let me know if there is anything I can do in that regard @maukoquiroga 🙂 |
@MattiSG As soon as circleci passes, this should be LGTM :) |
0a041eb
to
df0f412
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New changes reviewed & approved 😉
My previous review was auto-dismissed (cf. #990 (comment)).
df0f412
to
26bf3a7
Compare
d77299d
to
32e5ab1
Compare
Co-authored-by: Mahdi Ben Jelloul <mahdi.benjelloul@gmail.com>
32e5ab1
to
def6bf0
Compare
This stills needs your review @benjello 😃 |
There was a weird issue in circleci regarding the version check, however I cannot understand the cause. All looks good to me. |
Thanks @benjello ! |
Fixes #1230
Follows discussion on #1033
New Features
openfisca_core.types
Documentation