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

Improve logging #42

Merged
merged 3 commits into from
Jan 6, 2024
Merged

Improve logging #42

merged 3 commits into from
Jan 6, 2024

Conversation

teutoburg
Copy link
Contributor

@teutoburg teutoburg commented Jan 5, 2024

Correct loggers

Until now, our logging here used the root logger, which is the result of simply running logging.warning()/error()/etc without any other configuration. This is not ideal for a number of reasons and also "strongly advised" against in the docs. There are now three separate Loggers, one for each module (core and ui) and one top level logger for the package. As per the same paragraph in said docs, I included a NullHandler to the top logger.

Interoperability

In line with the planned changes in our other packages, all logger names start with "astar.", so it will be easy to collect all logging from all our packages if used together in the future.

@teutoburg teutoburg self-assigned this Jan 5, 2024
Copy link

codecov bot commented Jan 5, 2024

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (cf8392e) 75.00% compared to head (70d8885) 75.34%.

Files Patch % Lines
skycalc_ipy/core.py 20.00% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #42      +/-   ##
==========================================
+ Coverage   75.00%   75.34%   +0.34%     
==========================================
  Files           3        3              
  Lines         360      365       +5     
==========================================
+ Hits          270      275       +5     
  Misses         90       90              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@teutoburg teutoburg added enhancement New feature or request API How users interact with the software labels Jan 5, 2024
@teutoburg
Copy link
Contributor Author

Codecov diff hit fails because many of the logging calls here are triggered by conditions which are not tested 🙄

@teutoburg teutoburg marked this pull request as ready for review January 5, 2024 22:16
@teutoburg
Copy link
Contributor Author

Still TODO

As the name suggests, skycalc_ipy is intended to (also) be used from interactive IPython sessions. In this use case, it is unlikely that logging will be configured externally, but we might still want to give the user an easy option to receive logging messages. For the future, I thus propose to:

  • Add a "verbose" option to SkyCalc
    This would internally configure a console handler for our loggers. We might want to set this to True by default.
  • Allow different levels of verbosity
  • Add more (info, debug) logging for normal operation

And eventually maybe

  • Split the interface for interactive use and internal use
    Maybe this is already done with ui.get_almanac_data() in some places? Anyway, if the function/class used when skycalc_ipy is imported as a library in the background (e.g. in ScopeSim) is different from the one used interactively, the verbosity options discussed above might be easier to configure.

@teutoburg teutoburg requested a review from hugobuddel January 5, 2024 22:30
Copy link
Contributor

@hugobuddel hugobuddel left a comment

Choose a reason for hiding this comment

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

Good!

I don't think we have to spend much effort into improving the logging of skycalc_ipy per se. However, I think your goal is to improve logging in all our projects, and from that perspective it is good to experiment here first.

@hugobuddel hugobuddel self-requested a review January 6, 2024 20:33
@teutoburg
Copy link
Contributor Author

However, I think your goal is to improve logging in all our projects, and from that perspective it is good to experiment here first.

Indeed that was the idea 🙂

@teutoburg
Copy link
Contributor Author

Urgh, now this has conflicts because I merged #43 first

@teutoburg teutoburg merged commit dddd9a9 into master Jan 6, 2024
14 of 15 checks passed
@teutoburg teutoburg deleted the fh/logging branch January 6, 2024 20:48
@hugobuddel
Copy link
Contributor

Urgh, now this has conflicts because I merged #43 first

Sounds like you were fhlogging yourself!

Nooooo... I'll let myself out. It's Monday..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API How users interact with the software enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants