Skip to content

chore: remove unneeded logging and code marked unreachable that has not been reached. - #4305

Merged
rjsparks merged 2 commits into
ietf-tools:mainfrom
rjsparks:logging
Aug 4, 2022
Merged

chore: remove unneeded logging and code marked unreachable that has not been reached.#4305
rjsparks merged 2 commits into
ietf-tools:mainfrom
rjsparks:logging

Conversation

@rjsparks

@rjsparks rjsparks commented Aug 1, 2022

Copy link
Copy Markdown
Member

No description provided.

@codecov

codecov Bot commented Aug 1, 2022

Copy link
Copy Markdown

Codecov Report

Merging #4305 (f647045) into main (fe1f51a) will increase coverage by 0.05%.
The diff coverage is 92.17%.

❗ Current head f647045 differs from pull request most recent head b99ee1a. Consider uploading reports for the commit b99ee1a to get more accurate results

@@            Coverage Diff             @@
##             main    #4305      +/-   ##
==========================================
+ Coverage   88.40%   88.46%   +0.05%     
==========================================
  Files         294      294              
  Lines       39197    39186      -11     
==========================================
+ Hits        34652    34665      +13     
+ Misses       4545     4521      -24     
Impacted Files Coverage Δ
ietf/api/urls.py 100.00% <ø> (ø)
ietf/api/views.py 88.88% <ø> (-0.18%) ⬇️
ietf/community/models.py 89.09% <ø> (ø)
ietf/doc/urls.py 100.00% <ø> (ø)
ietf/nomcom/forms.py 95.39% <ø> (ø)
ietf/nomcom/models.py 91.73% <ø> (ø)
ietf/secr/meetings/views.py 82.82% <ø> (ø)
ietf/stats/views.py 88.75% <ø> (-0.05%) ⬇️
ietf/utils/markdown.py 100.00% <ø> (ø)
ietf/group/views.py 90.83% <40.00%> (-0.08%) ⬇️
... and 22 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

@rjsparks rjsparks changed the title chore: remove unneeded logging and code marked unreachable that has n… chore: remove unneeded logging and code marked unreachable that has not been reached. Aug 1, 2022

@jennifer-richards jennifer-richards left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A couple small comments inline. Probably should remove the empty save() method but the other is just a thought.

Comment thread ietf/doc/models.py Outdated
Comment thread ietf/doc/views_doc.py
prev_rev = doc.rev
# not sure what to do if non-numeric values come back, so at least log it
log.assertion('doc.rfc_number().isdigit()')
log.assertion('doc.rfc_number().isdigit()') # .rfc_number() is expensive...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Expensive, but at least cached. At the moment the value is used again in real code immediately after - could catch exceptions there instead of asserting ahead of time to reduce the risk that the assertions outlive the code they protect.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Well, as you note, it's not clear what to do if the expectations of those functions are violated, and I view the assertions as a short term safety net checking that we didn't have broken data. We could probably just pull the asserts, but since they aren't really adding load, leaving them for awhile longer won't hurt us.

@rjsparks
rjsparks merged commit 9c236ea into ietf-tools:main Aug 4, 2022
@rjsparks
rjsparks deleted the logging branch August 4, 2022 16:25
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Aug 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants