openfisca / openfisca-core

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

Fix docstrings #1131

Closed bonjourmauko closed 2 years ago

bonjourmauko commented 2 years ago

https://github.com/openfisca/openfisca-doc/pull/264

@MattiSG

coveralls commented 2 years ago

Coverage Status

Coverage increased (+0.02%) to 78.944% when pulling 60c4421e9778eb3974bb92e18c7b9b6eb60646aa on fix-docstring into 7c564231ffb43be433efa1270b1f8cbc9b6c7951 on master.

MattiSG commented 2 years ago

This PR indeed fixes most issues that were uncovered by updating Sphinx in https://github.com/openfisca/openfisca-doc/pull/264. The only one left at this stage is a very mysterious <unknown>:1: WARNING: py:obj reference target not found: openfisca_core.types.data_types.arrays.T. More details in https://github.com/openfisca/openfisca-doc/pull/264#issuecomment-1157868878.

MattiSG commented 2 years ago

I removed the py:obj reference target not found: warning by removing the nitpicky option in Sphinx. This hides it but does not make it go away entirely. See more in https://github.com/openfisca/openfisca-doc/pull/264#issuecomment-1157876485.

There are now problems detected by linting files in Core, that are not detected when building the doc:

openfisca_core/parameters/parameter.py:50: error: Incompatible types in assignment (expression has type "None", variable has type "str")
openfisca_core/parameters/parameter.py:52: error: Incompatible types in assignment (expression has type "None", variable has type "str")
openfisca_core/parameters/parameter.py:59: error: Incompatible types in assignment (expression has type "Optional[Any]", variable has type "str")
openfisca_core/parameters/parameter.py:67: error: Incompatible types in assignment (expression has type "Optional[Any]", variable has type "str")
openfisca_core/taxbenefitsystems/tax_benefit_system.py:52: error: Need type annotation for "_parameters_at_instant_cache" (hint: "_parameters_at_instant_cache: Dict[<type>, <type>] = ...")
openfisca_core/taxbenefitsystems/tax_benefit_system.py:53: error: Need type annotation for "variables" (hint: "variables: Dict[<type>, <type>] = ...")
openfisca_core/taxbenefitsystems/tax_benefit_system.py:54: error: Need type annotation for "open_api_config" (hint: "open_api_config: Dict[<type>, <type>] = ...")
openfisca_core/reforms/reform.py:76: error: "None" has no attribute "parameters"

Many of those points, while relevant, seem to have potential major implications, such as setting the default description of a Parameter to "" instead of None. I don't feel experienced enough in Python docstrings to fully understand the consequences.

MattiSG commented 2 years ago

I managed to make the compiler happy on all those items. However, the type hints I gave were often too wide, and some I purely removed as I did not know what to set. This is not a satisfactory nor relevant use of a static checker. Considering the added friction and the lack of documentation on how to handle such cases, I am in favour of removing type checks altogether.

MattiSG commented 2 years ago

This PR was opened by @maukoquiroga but I edited it too much to be a relevant reviewer. A third-party review would be very welcome.

MattiSG commented 2 years ago

Thanks @benjello for your help! If you did not spot any issue during review yet prefer not to approve, I will approve this PR myself so we can unblock the documentation compilation.