-
Notifications
You must be signed in to change notification settings - Fork 95
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
Type hints & misc smaller fixes #2976
Conversation
IMO it's a bit weird to convert only one of the |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #2976 +/- ##
==========================================
+ Coverage 28.22% 28.36% +0.14%
==========================================
Files 85 85
Lines 14634 14664 +30
==========================================
+ Hits 4131 4160 +29
- Misses 10503 10504 +1
☔ View full report in Codecov by Sentry. |
That's the one I cared about 🤷 |
e4de8b9
to
aa35b9d
Compare
|
afe528f
to
ba38c7d
Compare
Fabian Vogt ***@***.***> writes:
@Vogtinator commented on this pull request.
> """Convert an XML element comment into a dictionary.
:param comment_element: XML element that store a comment.
:returns: A Python dictionary object.
"""
- comment = {
+ when_str = comment_element.get('when')
In that case I recommend to remove the handling for `None` again. If the value is somehow missing it should fail fast and not at some point later when there's an unexpected missing timestamp somewhere...
Done.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but let's merge on a less read-only day of the week.
@dcermak rebase please |
Done |
as mentioned on slack, it broke check_tags_in_requests.py:
|
For reference: this no longer works with osc 0.182.1 from Leap |
also with #2990? |
|
No description provided.