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

Add option to log messages to terminal #358

Merged
merged 8 commits into from
May 18, 2023
Merged

Add option to log messages to terminal #358

merged 8 commits into from
May 18, 2023

Conversation

Schobs
Copy link
Member

@Schobs Schobs commented Apr 3, 2023

Fixes #{issue_number}.

Description

Adds option to log to terminal. THis is useful since we want to avoid print statements, and in interactive examples it is important to print to terminal. This will not affect any existing code since it defaults to False.

Status

Ready

If you've been given access to pykale with role Triage or above, on the right (delete these after selection):

  • Select one most appropriate label.
  • When your pull request is ready for review, select a reviewer. Use the suggested one if unsure.

If you cannot see an option to select a reviewer/label, that means one maintainer will get notified upon your pull request and then select for you.

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Breaking change (fix or new feature that would cause existing functionality to change).
  • New tests added to cover the changes.
  • In-line docstrings updated.
  • Source for documentation at docs manually updated for new API.

@Schobs Schobs added the new feature New feature/module (including request) label Apr 3, 2023
@github-actions github-actions bot added this to In progress in v0.2.0 Apr 3, 2023
@codecov-commenter
Copy link

codecov-commenter commented Apr 3, 2023

Codecov Report

Merging #358 (7003d78) into main (90263d3) will decrease coverage by 0.07%.
The diff coverage is 33.33%.

📣 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             @@
##             main     #358      +/-   ##
==========================================
- Coverage   91.38%   91.31%   -0.07%     
==========================================
  Files          48       48              
  Lines        5130     5135       +5     
==========================================
+ Hits         4688     4689       +1     
- Misses        442      446       +4     
Impacted Files Coverage Δ
kale/utils/logger.py 83.33% <33.33%> (-16.67%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Mdnaimulislam
Mdnaimulislam previously approved these changes Apr 6, 2023
Copy link
Collaborator

@Mdnaimulislam Mdnaimulislam left a comment

Choose a reason for hiding this comment

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

@Schobs Didn't pass all the checks.

@Mdnaimulislam Mdnaimulislam self-requested a review April 8, 2023 11:30
@Mdnaimulislam Mdnaimulislam dismissed their stale review April 8, 2023 11:32

checks not succesfull after updating the branch.

Copy link
Collaborator

@Mdnaimulislam Mdnaimulislam left a comment

Choose a reason for hiding this comment

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

All okay.

kale/utils/logger.py Outdated Show resolved Hide resolved
Copy link
Member

@haipinglu haipinglu left a comment

Choose a reason for hiding this comment

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

@Schobs Some minor changes needed before merging.

@Mdnaimulislam Mdnaimulislam self-requested a review May 4, 2023 14:41
Copy link
Collaborator

@Mdnaimulislam Mdnaimulislam left a comment

Choose a reason for hiding this comment

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

test_logger is coveraging 100% of logger. LGTM. Well done.

@haipinglu haipinglu changed the title added option to log messages to terminal. default is false Add option to log messages to terminal May 18, 2023
@haipinglu haipinglu enabled auto-merge May 18, 2023 11:12
@haipinglu haipinglu merged commit 7ba8e4f into main May 18, 2023
7 checks passed
@haipinglu haipinglu deleted the logger_update branch May 18, 2023 11:49
This was referenced Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature/module (including request)
Projects
Status: Done
v0.2.0
In progress
Development

Successfully merging this pull request may close these issues.

None yet

4 participants