Codecov Report

Merging #129 (38d6111) into master (0679bc2) will increase coverage by 0.02%. The diff coverage is 87.35%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master     #129      +/-   ##
==========================================
+ Coverage   92.30%   92.32%   +0.02%     
==========================================
  Files         114      114              
  Lines        3730     3728       -2     
==========================================
- Hits         3443     3442       -1     
+ Misses        287      286       -1

Impacted Files	Coverage Δ
pynsee/localdata/get_geo_list.py	`90.32% <ø> (ø)`
pynsee/localdata/get_old_city.py	`80.95% <ø> (ø)`
pynsee/macrodata/_dwn_idbank_files.py	`94.52% <ø> (ø)`
pynsee/macrodata/_get_idbank_internal_data.py	`88.46% <ø> (ø)`
pynsee/utils/_get_credentials.py	`73.91% <20.00%> (ø)`
pynsee/download/_load_data_from_schema.py	`69.81% <33.33%> (ø)`
pynsee/localdata/get_area_list.py	`94.00% <66.66%> (ø)`
pynsee/macrodata/get_dataset_list.py	`88.63% <66.66%> (ø)`
pynsee/localdata/get_local_data.py	`85.91% <80.00%> (ø)`
pynsee/download/_check_url.py	`90.74% <83.33%> (ø)`
... and 74 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

tgrandje commented 1 year ago

Hello. Two quick questions about this PR :

should we expressly set the logging level in the module ? As you systematically substitued logging.info to each print, I think it undermines the whole purpose of it (which was about pynsee beeing too verbose) ?
should'nt we switch logging.info to logging.warning / logging.error in some cases ? I agree pynsee is too verbose, but some message should be displayed in most cases (for instance "f"File not found on insee.fr:\n{url}" "Please open an issue on:\nhttps://github.com/InseeFrLab/pynsee").

By the way, I'm thinking it may be a good opportunity to rethink some "multilines" comments in "one-liners". It would keep the logs concise (for instance to continue with the previous example, something like "f"File not found on insee.fr:\n{url}. Please open an issue on: https://github.com/InseeFrLab/pynsee") and easily readable.

tfardet commented 1 year ago

I agree with @tgrandje : "info" should only be used for things that are part of the normal workflow. Anything unusual should at least be "warning" and things related to files not found, data used instead of another, or anything saying people should contact the maintainer or raise an issue here should be "critical" IMO.

tgrandje commented 1 year ago

@lgeo3 I just did a PR on your own PR to try to set the levels in accordance with the current discussion (feel free to change it!) .

This is my firt PR on PR, I hope I've been doing this according to the rule: if not, let me know and I'll try to do a direct PR from my repo.

hadrilec commented 1 year ago

I would like to make a new release in the coming days, @lgeo3 could you please review the PR on your fork and let me know if the code is ready to be published or if further fine tuning is needed

tgrandje commented 1 year ago

@hadrilec @tfardet I just moved the PR to the main repo (through https://github.com/InseeFrLab/pynsee/pull/135)

tfardet commented 1 year ago

Closing due to inactivity in favor of #135

InseeFrLab / pynsee

feat: use logging to replace print #117 #129

Codecov Report