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

Adding Logger to SunPy #2980

Merged
merged 33 commits into from Mar 20, 2019

Conversation

Projects
4 participants
@ehsteve
Copy link
Member

commented Mar 12, 2019

Description

This PR addresses issue #1859 by adding a logger based on Astropy's logger. It adds the ability to configure the logger within the sunpy configuration file.

Fixes #1859.

To do

  • add supporting documentation for users
  • add supporting documentation and standards for developers
  • add changelog
  • add tests

@ehsteve ehsteve added the [WIP] label Mar 12, 2019

@ehsteve ehsteve added this to the 1.0 milestone Mar 12, 2019

@pep8speaks

This comment has been minimized.

Copy link

commented Mar 12, 2019

Hello @ehsteve! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 155:101: E501 line too long (123 > 100 characters)

Line 208:101: E501 line too long (107 > 100 characters)

Line 127:101: E501 line too long (104 > 100 characters)

Line 413:101: E501 line too long (113 > 100 characters)
Line 431:101: E501 line too long (109 > 100 characters)
Line 552:101: E501 line too long (110 > 100 characters)
Line 571:101: E501 line too long (106 > 100 characters)
Line 590:101: E501 line too long (107 > 100 characters)
Line 591:40: E127 continuation line over-indented for visual indent
Line 610:101: E501 line too long (114 > 100 characters)
Line 789:101: E501 line too long (111 > 100 characters)

Line 75:101: E501 line too long (102 > 100 characters)

Line 85:101: E501 line too long (112 > 100 characters)
Line 88:101: E501 line too long (113 > 100 characters)

Line 8:101: E501 line too long (103 > 100 characters)
Line 31:101: E501 line too long (108 > 100 characters)

Line 88:101: E501 line too long (108 > 100 characters)

Line 9:44: E231 missing whitespace after ':'
Line 36:101: E501 line too long (115 > 100 characters)

Comment last updated at 2019-03-20 11:53:06 UTC
@sunpy-bot

This comment has been minimized.

Copy link

commented Mar 12, 2019

Thanks for the pull request @ehsteve! Everything looks great!

2 similar comments
@sunpy-bot

This comment has been minimized.

Copy link

commented Mar 12, 2019

Thanks for the pull request @ehsteve! Everything looks great!

@sunpy-bot

This comment has been minimized.

Copy link

commented Mar 12, 2019

Thanks for the pull request @ehsteve! Everything looks great!

@ehsteve ehsteve requested review from dpshelio, nabobalis and Cadair Mar 12, 2019

@Cadair
Copy link
Member

left a comment

This is awesome 🎉 thanks!

Does this correctly catch warnings in the logging output like astropy does? I think it would be great to have warnings filtering with the logger in the docs :)

For the tests I am sure you plan to add 😝 , you can use the caplog fixture in pytest to capture and assert logging entries.

Show resolved Hide resolved sunpy/__init__.py
Show resolved Hide resolved sunpy/__init__.py Outdated
Show resolved Hide resolved sunpy/__init__.py Outdated
Show resolved Hide resolved sunpy/__init__.py Outdated
Show resolved Hide resolved sunpy/roi/chaincode.py Outdated
@nabobalis

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

I will have to try this out but is that the only places we use print?

@ehsteve

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2019

@nabobalis I could not find many places that we use print at all! But maybe I wasn't searching well?

@ehsteve

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2019

@Cadair about tests. I thought about "stealing" the astropy tests but those are to ensure that the object does not error. We don't need that stuff right?

@ehsteve

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2019

Note that I had to move to using the RawConfigParser which means we can't do fancy config stuff but we weren't using any of that functionality now anyway.

@nabobalis

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

That is fine, I haven't searched myself, I just had thought we used it in more places.

We can have a look the astropy tests and "steal" what we want.

@ehsteve

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2019

@Cadair @nabobalis I updated to make sure we can catch warnings but only if we use the SunpyWarnings which is not used much at all. Do you want me to expand the scope of this PR and clean up everywhere to use SunpyWarnings?

@Cadair

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

I thought about "stealing" the astropy tests but those are to ensure that the object does not error. We don't need that stuff right?

We only need to test stuff we implement not import. We should however at the very least add a few tests to ensure the logging is working as expected and obeying the config. Also we should add a test for the warnings -> log code path as well.

Do you want me to expand the scope of this PR and clean up everywhere to use SunpyWarnings?

I think this probably makes sense?

Show resolved Hide resolved sunpy/util/logger.py Outdated
Show resolved Hide resolved sunpy/util/logger.py Outdated
Show resolved Hide resolved sunpy/data/sunpyrc Outdated
Show resolved Hide resolved sunpy/util/logger.py Outdated
Show resolved Hide resolved sunpy/data/sunpyrc Outdated
Show resolved Hide resolved sunpy/data/sunpyrc Outdated
Show resolved Hide resolved sunpy/data/sunpyrc Outdated
@Cadair
Copy link
Member

left a comment

Many inline comments, I have tried to go through and add suggestions for you in as many places as I can.

I am fine to let most of the warning changes through, there are a couple of places where they 100% need changing back.

Show resolved Hide resolved sunpy/data/sunpyrc Outdated
Show resolved Hide resolved sunpy/physics/transforms/differential_rotation.py Outdated
Show resolved Hide resolved sunpy/timeseries/metadata.py Outdated
Show resolved Hide resolved sunpy/timeseries/sources/eve.py Outdated
Show resolved Hide resolved sunpy/timeseries/timeseriesbase.py Outdated
Show resolved Hide resolved sunpy/util/decorators.py
Show resolved Hide resolved sunpy/util/logger.py Outdated
Show resolved Hide resolved sunpy/util/multimethod.py Outdated
Show resolved Hide resolved sunpy/net/vso/vso.py Outdated
Show resolved Hide resolved sunpy/map/mapbase.py Outdated
@Cadair

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

Also update your branch before I review it again 😁

Show resolved Hide resolved sunpy/util/exceptions.py Outdated
Show resolved Hide resolved docs/guide/logger.rst Outdated
Show resolved Hide resolved docs/guide/index.rst Outdated
Show resolved Hide resolved docs/dev_guide/logger.rst Outdated
Show resolved Hide resolved docs/dev_guide/logger.rst Outdated
Show resolved Hide resolved docs/guide/logger.rst Outdated
Show resolved Hide resolved docs/guide/logger.rst Outdated
Show resolved Hide resolved docs/guide/logger.rst Outdated
Show resolved Hide resolved docs/guide/logger.rst Outdated
Show resolved Hide resolved docs/guide/logger.rst Outdated

@Cadair Cadair added this to High Priority Features in SunPy 1.0 Mar 15, 2019

ehsteve and others added some commits Mar 13, 2019

Apply suggestions from code review
Co-Authored-By: ehsteve <ehsteve@users.noreply.github.com>
Apply suggestions from code review
Co-Authored-By: ehsteve <ehsteve@users.noreply.github.com>

@Cadair Cadair force-pushed the ehsteve:logger branch from da44cc1 to a740599 Mar 20, 2019

@Cadair Cadair merged commit 6efafa7 into sunpy:master Mar 20, 2019

16 checks passed

ci/circleci: 32bit Your tests passed on CircleCI!
Details
ci/circleci: egg-info-36 Your tests passed on CircleCI!
Details
ci/circleci: egg-info-37 Your tests passed on CircleCI!
Details
ci/circleci: figure-tests-36 Your tests passed on CircleCI!
Details
ci/circleci: html-docs Your tests passed on CircleCI!
Details
ci/circleci: pip-install Your tests passed on CircleCI!
Details
codecov/patch 87.09% of diff hit (target 85.79%)
Details
codecov/project 85.85% (+0.05%) compared to a09a03e
Details
giles Click details to preview the documentation build
Details
sunpy-bot All checks passed
sunpy.sunpy Build #20190320.8 succeeded
Details
sunpy.sunpy (Linux_36_Conda_offline) Linux_36_Conda_offline succeeded
Details
sunpy.sunpy (Linux_36_offline) Linux_36_offline succeeded
Details
sunpy.sunpy (Linux_37_online) Linux_37_online succeeded
Details
sunpy.sunpy (Windows_36_offline) Windows_36_offline succeeded
Details
sunpy.sunpy (macOS_37_offline) macOS_37_offline succeeded
Details
@Cadair

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

Nice one @ehsteve !! I hope you don't mind me cleaning it up a little at the end 😀

@ehsteve ehsteve deleted the ehsteve:logger branch Mar 20, 2019

@Cadair Cadair moved this from High Priority Features to Finished in SunPy 1.0 Mar 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.