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

Fix log-level option #194

Merged
merged 1 commit into from
Oct 30, 2020
Merged

Conversation

ltm
Copy link
Contributor

@ltm ltm commented Oct 28, 2020

The log level is set on the logger instance in icloudpd.base.main based on the --log-level option, but subsequent calls to setup_logger() resets the level to DEBUG. As a result the --log-level option is effectively ignored.

Since the --log-level option defaults to DEBUG it's safe to remove logger.setLevel() from setup_logger() which allows the option to work as expected.

@menkej
Copy link
Collaborator

menkej commented Oct 28, 2020

Hi @ltm !
Thanks for your pull request! The change looks rather simple. I'll test it later.

Can you also add a test case that checks the logging handling?

@ltm
Copy link
Contributor Author

ltm commented Oct 29, 2020

@menkej I've updated the existing test_log_levels test to check for the appropriate levels in the output. For some reason an unrelated test (i.e. test_until_found) has started failing, but I see it's failing in other PRs as well so I think it's unrelated to my changes.

Copy link
Collaborator

@menkej menkej left a comment

Choose a reason for hiding this comment

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

Thanks updating the test cases! I'l request one additional check: Both tests assume the logger running on INFO. There should also be a test for DEBUG level as well, what do you think?

@menkej
Copy link
Collaborator

menkej commented Oct 29, 2020

test_until_found is an issue of its own. It behaves inconsistent (I think we have a PR for that already). I have no Idea why it sometimes runs fine and sometimes does not...

@ltm
Copy link
Contributor Author

ltm commented Oct 29, 2020

@menkej Makes sense. I've updated (and refactored) the test to check all three log levels.

Copy link
Collaborator

@menkej menkej left a comment

Choose a reason for hiding this comment

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

Nice work @ltm! Way better than before. Last little cream on the cake is to add this fix to the CHANGELOG.md in the unreleased section as "fix:" and then we're ready to merge! Sorry I forgot to mention this before. We just introduced the CHANGELOG recently and I'm still not used to it... ;-)

@ltm
Copy link
Contributor Author

ltm commented Oct 30, 2020

@menkej No problem. I've added a changelog entry.

@menkej menkej merged commit 988788f into icloud-photos-downloader:master Oct 30, 2020
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