Skip to content

Simple logging system#47

Merged
paulmueller merged 10 commits intoAFM-analysis:masterfrom
PinkShnack:30_log_logging_system
Apr 9, 2026
Merged

Simple logging system#47
paulmueller merged 10 commits intoAFM-analysis:masterfrom
PinkShnack:30_log_logging_system

Conversation

@PinkShnack
Copy link
Copy Markdown
Contributor

@PinkShnack PinkShnack commented Apr 7, 2026

As outlined in #30, we should have a logging system for when files don't open correctly.

Features

  • The logging system will output to a temporary afmformats.log file in the temp folder.
    • this can be picked up by github actions as an artifact for ci debugging
  • %s string formatting used to keep with py3.6
  • unit test for debugger
  • show screenshots of local file, terminal output and ci artifact

@PinkShnack PinkShnack marked this pull request as draft April 7, 2026 11:40
@PinkShnack
Copy link
Copy Markdown
Contributor Author

CI screenshots showing:
artifacts location
image

debug file (downloaded locally, looks like lots of interesting things to fix... displayed using TailViewer)
image

@PinkShnack
Copy link
Copy Markdown
Contributor Author

Screenshot of local debug examples (just python console - found error with a docs example!)

image

@PinkShnack PinkShnack marked this pull request as ready for review April 8, 2026 09:51
@PinkShnack PinkShnack requested a review from paulmueller April 8, 2026 09:51
@paulmueller
Copy link
Copy Markdown
Member

Thanks for the PR!

IMO creating log files automatically on import without the user doing anything else seems a bit invasive. I would propose to remove all calls to configure_logging() in the library and instead setup logging during pytest_configure. Downstream users (which also includes PyJibe) will likely just attach their own handlers to the afmformats logger, so a default afmformats.log FileHandler will be redundant.

Some additional comments:

  • configure_logging() is called twice (in __init__.py and in logging_setup.py); such redundancies should be avoided
  • The DEFAULT_LOG_PATH, as it is defined, is not a default log path, because it will be either AFMFORMATS_LOG_PATH or /tmp (on POSIX). I would suggest to define it directly as os.path.join(tempfile.gettempdir(), "afmformats.log") and then use it in configure_logging.

@PinkShnack
Copy link
Copy Markdown
Contributor Author

PinkShnack commented Apr 8, 2026

Thanks for the PR!

IMO creating log files automatically on import without the user doing anything else seems a bit invasive.

I hadn't even considered this aspect! Thanks

I would propose to remove all calls to configure_logging() in the library and instead setup logging during pytest_configure. Downstream users (which also includes PyJibe) will likely just attach their own handlers to the afmformats logger, so a default afmformats.log FileHandler will be redundant.

I'll do that, and perhaps include in the docs that users can set configure_logging() when they want logging output + logged file. That way I'll combine the configure_logging and configure_console_logging.

Some additional comments:

  • configure_logging() is called twice (in __init__.py and in logging_setup.py); such redundancies should be avoided

Thanks will do.

  • The DEFAULT_LOG_PATH, as it is defined, is not a default log path, because it will be either AFMFORMATS_LOG_PATH or /tmp (on POSIX). I would suggest to define it directly as os.path.join(tempfile.gettempdir(), "afmformats.log") and then use it in configure_logging.

👍

@PinkShnack
Copy link
Copy Markdown
Contributor Author

@paulmueller whenever you have time please check that this makes sense. Perhaps the debugging level at different points of the loading functions could be improved?

Copy link
Copy Markdown
Member

@paulmueller paulmueller left a comment

Choose a reason for hiding this comment

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

Looks good to me otherwise.

@PinkShnack PinkShnack changed the title Draft: simple logging system Simple logging system Apr 9, 2026
@PinkShnack
Copy link
Copy Markdown
Contributor Author

Ready

@paulmueller paulmueller merged commit 1778ebc into AFM-analysis:master Apr 9, 2026
9 checks passed
@PinkShnack PinkShnack deleted the 30_log_logging_system branch April 9, 2026 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants