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

feat: logging 0 - add logger configuration and update logging in step 1 #138

Merged
merged 43 commits into from
Mar 18, 2024

Conversation

hollandjg
Copy link
Member

@hollandjg hollandjg commented Feb 29, 2024

Hi all, here's a draft of a possibility to add a logger.

The benefits of redirecting the output to stdout are definitely the speed and ease, but I'm worried that it's a bit hacky and might make life less understandable for future developers.
This way gives us a bit more gradation in the output we want or not – the default also hides all the output, but adding --debug or --verbose directly after the icesat2waves like this:

icesat2waves --debug load-file --track-name 20190502052058_05180312_005_01 --batch-key SH_testSLsinglefile2 --output-dir ./output 

... will give more output.

Things which are needed to complete this are (for each file in src/):

  • Add import logging
  • Add a _logger = logging.getLogger(__name__) after the imports
  • replace print and echo with _logger.debug everywhere
  • For files you might run manually and want to see the info and debug outputs, add
    if __name__ == "__main__":
        logging.basicConfig(level=logging.WARNING)  # <-- this line
        load_file_app()
    ... which will hide info and debug outputs, and just show any warnings.

@cpaniaguam cpaniaguam self-requested a review March 1, 2024 18:45
@hollandjg hollandjg marked this pull request as ready for review March 4, 2024 18:51
@hollandjg hollandjg changed the title feat: add logger configuration and upate logging in step 1 feat: add logger configuration and update logging in step 1 Mar 4, 2024
@hollandjg
Copy link
Member Author

There are some std output we can do as a feedback to the user on what is happening. For example that the operation finished, or file was saved.

We can totally do that, but I'd do that in a separate PR – this set is large enough and I want to minimize the merge conflicts. But adding an info at the start and the end of each main function in the analysis directory would be neat.

cpaniaguam
cpaniaguam previously approved these changes Mar 5, 2024
Copy link
Member

@cpaniaguam cpaniaguam left a comment

Choose a reason for hiding this comment

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

👍

kmilo9999
kmilo9999 previously approved these changes Mar 6, 2024
Copy link
Collaborator

@kmilo9999 kmilo9999 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

@hollandjg hollandjg dismissed stale reviews from cpaniaguam and kmilo9999 via b22e178 March 11, 2024 19:56
# Conflicts:
#	src/icesat2waves/analysis_db/B01_SL_load_single_file.py
#	src/icesat2waves/local_modules/m_colormanager_ph3.py
#	src/icesat2waves/local_modules/m_general_ph3.py
Copy link
Member

@cpaniaguam cpaniaguam left a comment

Choose a reason for hiding this comment

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

Thanks for your contributions to this project, John!

I pointed out some files that are not uniformly formatted and a class that is not named according to PEP8 -- I guess it slipped through the cracks.

Copy link
Member

Choose a reason for hiding this comment

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

This file does not seem to be uniformly formatted. Also, the color class is not named according to PEP8.

Copy link
Member Author

@hollandjg hollandjg Mar 14, 2024

Choose a reason for hiding this comment

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

I've reformatted everything in a follow-on PR #198 – I don't want to deal with the merge conflicts or increase the number of changes here.

Copy link
Member Author

@hollandjg hollandjg Mar 14, 2024

Choose a reason for hiding this comment

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

The color class is indeed not named according to PEP8. Not in scope for this PR – I'll open an issue.

Copy link
Collaborator

@kmilo9999 kmilo9999 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

@hollandjg hollandjg merged commit d91d987 into main Mar 18, 2024
2 checks passed
@hollandjg hollandjg deleted the feat-add-logger branch March 18, 2024 19:40
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.

3 participants