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

Remove logging abstraction to facilitate debugging and reduce bloat. #109

Merged
merged 5 commits into from
Dec 1, 2020

Conversation

sobolevnrm
Copy link
Collaborator

Also performed minor delinting.

Fixes #108

@codecov
Copy link

codecov bot commented Nov 30, 2020

Codecov Report

Merging #109 (e081bd5) into master (f3bbd80) will increase coverage by 0.03%.
The diff coverage is 60.10%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #109      +/-   ##
==========================================
+ Coverage   73.27%   73.31%   +0.03%     
==========================================
  Files          24       24              
  Lines        3982     3987       +5     
==========================================
+ Hits         2918     2923       +5     
  Misses       1064     1064              
Impacted Files Coverage Δ
propka/parameters.py 62.80% <21.73%> (+0.15%) ⬆️
propka/ligand_pka_values.py 26.08% <25.00%> (+3.55%) ⬆️
propka/coupled_groups.py 76.10% <28.57%> (+0.15%) ⬆️
propka/bonds.py 60.95% <30.00%> (+0.15%) ⬆️
propka/run.py 63.88% <33.33%> (-7.08%) ⬇️
propka/lib.py 65.71% <50.00%> (-2.50%) ⬇️
propka/version.py 63.70% <57.14%> (+0.27%) ⬆️
propka/group.py 77.26% <64.70%> (+0.03%) ⬆️
propka/hydrogens.py 15.76% <66.66%> (+0.46%) ⬆️
propka/molecular_container.py 92.59% <75.00%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3bbd80...e081bd5. Read the comment docs.

Copy link
Collaborator

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

Overall this looks good.

There's one message that was originally tagged "info" but is clearly "error" – see comment.

It's not clear to me if the default for logging is to show messages and at which level. There should be some user documentation for how to enable different levels. Similarly, is there a commandline option to change the logging level?

propka/ligand_pka_values.py Outdated Show resolved Hide resolved
propka/run.py Show resolved Hide resolved
_LOGGER.info('CYS-CYS %s', self.CYS_CYS_exception)

_LOGGER.info('--------------- Mapping -------------------------------')
_LOGGER.info("""
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like latex, meant to be copied and pasted. Is this used by anyone? If so, perhaps make it easier to grep it from the logs by prefixing every line with some marker such as "LATEX>" or somesuch??

It might not be worth any effort, though. Up to you.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You and @speleo3 know that I love to delete code... and I don't think anyone is using this. I would happily delete if you both agree.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's the same to me – I am pretty sure I never looked at it.

@sobolevnrm sobolevnrm merged commit 45aa08a into master Dec 1, 2020
@sobolevnrm sobolevnrm deleted the nathan/logging branch December 1, 2020 19:55
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.

Make logging more transparent and useful
3 participants