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

Eliminated the usage of astropy logging #799

Merged

Conversation

kr-2003
Copy link
Contributor

@kr-2003 kr-2003 commented Feb 11, 2024

Encapsulated the logging setup in a function in a separate file(loggingconfig.py), then called this function from different files where logging is needed.

@pep8speaks
Copy link

pep8speaks commented Feb 11, 2024

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-02-11 20:25:16 UTC

@kr-2003
Copy link
Contributor Author

kr-2003 commented Feb 11, 2024

@matteobachetti Kindly review this PR.

@matteobachetti
Copy link
Member

@kr-2003 could you post a screenshot of the new logger in action?

Copy link

codecov bot commented Feb 11, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (5285c23) 96.35% compared to head (004f1ae) 94.37%.

Files Patch % Lines
stingray/filters.py 66.66% 1 Missing ⚠️
stingray/pulse/accelsearch.py 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #799      +/-   ##
==========================================
- Coverage   96.35%   94.37%   -1.98%     
==========================================
  Files          43       44       +1     
  Lines        8784     8824      +40     
==========================================
- Hits         8464     8328     -136     
- Misses        320      496     +176     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kr-2003
Copy link
Contributor Author

kr-2003 commented Feb 11, 2024

Screenshot 2024-02-11 at 6 35 35 PM Screenshot 2024-02-11 at 7 13 16 PM

@kr-2003
Copy link
Contributor Author

kr-2003 commented Feb 11, 2024

@matteobachetti Most of the changes were related to logging.info.

@matteobachetti
Copy link
Member

@kr-2003 thanks for your contribution! Please note that there are tests failing related to the absence of a changelog (in towncrier format) and the formatting of files (using black). Please review the contribution workflow at https://docs.stingray.science/en/latest/contributing.html#contribution-workflow

@kr-2003
Copy link
Contributor Author

kr-2003 commented Feb 11, 2024

@matteobachetti I passed the tests related to towncrier and black. Now CI Tests / ci-tests (Slow tests on Linux, Py3.11, all deps and coverage, ubuntu-latest, 3.11, py311-test-all... (pull_request) Failing after 10m and codecov/project — 94.37% (-1.98%) compared to 5285c23 are failing.

@kr-2003
Copy link
Contributor Author

kr-2003 commented Feb 11, 2024

Actually logger.info is not getting coverage in two files.

Copy link
Member

@matteobachetti matteobachetti left a comment

Choose a reason for hiding this comment

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

LGTM. The failures and the lack of coverage are independent on this PR. Thanks @kr-2003 !

@matteobachetti matteobachetti added this pull request to the merge queue Feb 12, 2024
Merged via the queue into StingraySoftware:main with commit d47c95b Feb 12, 2024
15 of 17 checks passed
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