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.

Sign up for GitHub

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

Jump to bottom

Update logging messages and raising errors #731

Merged
merged 3 commits into from Apr 16, 2024
Merged

Update logging messages and raising errors #731

merged 3 commits into from Apr 16, 2024

Conversation

raoulcollenteur
Copy link
Member

@raoulcollenteur raoulcollenteur commented Apr 9, 2024 •
edited

Short Description

In this PR the logging and error raising is made similar throughout Pastas, plus I described how this is done (for now). Even if we want to change this in the future that will be easier because it is all consistent throughout Pastas now.

  • Use s-strings (logging module is optimized for that)
  • raise errors after logging (so script terminates, but error is captured)
  • Add description for developers in code_style.rst

See doc/developers/code_style.rst for a description of the proposed workflow for logging and raising errors.

Checklist before PR can be merged:

@raoulcollenteur raoulcollenteur added this to the 1.5 milestone Apr 9, 2024
@raoulcollenteur raoulcollenteur added enhancement Indicates improvement of existing features code quality Related to code quality, testing, CI, project setup etc. labels Apr 9, 2024
@codacy-production Codacy Production
Copy link

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
❌ -0.20% (target: +0.00%) ✅ 13.33%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (bf64e28) 6163 4647 75.40%
Head commit (1199278) 6198 (+35) 4661 (+14) 75.20% (-0.20%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#731) 75 10 13.33%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

You may notice some variations in coverage metrics with the latest Coverage engine update. For more details, visit the documentation

@raoulcollenteur
Copy link
Member Author

This PR is also ready for review.

@raoulcollenteur raoulcollenteur linked an issue Apr 15, 2024 that may be closed by this pull request
Establish and document deprecation protocol #546
Closed
3 tasks
@raoulcollenteur
Copy link
Member Author

Somebody time to review this PR? It would be nice to merge and then update the master branch :)

rubencalje
rubencalje approved these changes Apr 16, 2024
View reviewed changes
Copy link
Collaborator

@rubencalje rubencalje left a comment •
edited

Choose a reason for hiding this comment

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

I approve, looks good. I have one minor question: in the __init__ of pasatas we have just added a warning about the noisemodel using warnings.warn. Should we not change this to a logger.warning?

@raoulcollenteur
Copy link
Member Author

I approve, looks good. I have one minor question: in the __init__ of pasatas we have just added a warning about the noisemodel using warnings.warn. Should we not change this to a logger.warning?

I wanted to make 100% sure that warning is raised, even if someone has a log level of ERROR (dont even know if that is possible) so that's why for that one I used warn. But for other warnings, logger.warning should be it (unless we change that in the future).

Merging PR.

@raoulcollenteur raoulcollenteur merged commit c44c3e2 into dev Apr 16, 2024
11 of 13 checks passed
@raoulcollenteur raoulcollenteur deleted the update_errors branch April 16, 2024 18:47
@dbrakenhoff dbrakenhoff mentioned this pull request May 14, 2024
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rubencalje rubencalje rubencalje approved these changes

@dbrakenhoff dbrakenhoff Awaiting requested review from dbrakenhoff

@martinvonk martinvonk Awaiting requested review from martinvonk

@OnnoEbbens OnnoEbbens Awaiting requested review from OnnoEbbens

Assignees

@raoulcollenteur raoulcollenteur

Labels
code quality Related to code quality, testing, CI, project setup etc. enhancement Indicates improvement of existing features
Projects
None yet
Milestone
1.5
Development

Successfully merging this pull request may close these issues.

Establish and document deprecation protocol
2 participants
@raoulcollenteur @rubencalje