search

InseeFrLab / pynsee

pynsee package contains tools to easily search and download french data from INSEE and IGN APIs
https://pynsee.readthedocs.io/en/latest/
MIT License
67 stars 8 forks source link

Avoid strange name shadowing and simplify structure #174

Open tfardet opened 1 year ago

tfardet commented 1 year ago

At the moment, pynsee is written with one file per class or function, with the files having the same name. This seem to lead to name shadowing upon import (cf. #173) [1] and makes imports extremely cumbersome.

In my opinion, there is no good reason to have one file per function (especially not with the same name) as any good IDE will automatically locate definitions (even GitHub's web UI does this).

A simple way to do this with minimal changes would be to remove duplicate functions (no need to have get_geodata calling _get_geodata). So we can just keep _get_geodata.py which would contain get_geodata removing both one file, one function call, and the weird name shadowing issue.

For classes, we should follow the python convention and keep class files lowercase.

[1]: though this is not supposed to happen (declared variables are supposed to have precedence over modules) so I'm not really sure why this led to bugs in the CI

hadrilec commented 1 year ago

I agree that it is not really 'beautiful', the main reason of this split is to avoid circular dependencies. If you manage to put the functions next to another without any circular dependency then go for it!

tfardet commented 1 year ago

Roger that! I'm not really worried about the beauty so much as about this very puzzling import issue so if we can simplify import and make sure that we don't run into this anymore, it would be a plus.

I'll have a look at it whenever I can. In the meantime, I think we can at least move class files to lowercase, that won't hurt.