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

Increase stacklevel of timezone warning #1641

Merged
merged 7 commits into from
Feb 22, 2023
Merged

Conversation

CodyCBakerPhD
Copy link
Collaborator

@CodyCBakerPhD CodyCBakerPhD commented Feb 2, 2023

Motivation

I just ran across a confusing case during a conversion.

I'm setting both session_start_time and the subect's date_of_birth.

I intentionally set the tzinfo for the datetime of the session_start_time, but was very confused for while as to why I kept getting the warning from PyNWB about not having set a timezone.

Little did I know it was not coming from the session_start_time (which 90+% of the time is the cause of that warning), but it was from https://github.com/NeurodataWithoutBorders/pynwb/blob/dev/src/pynwb/file.py#L120 which does the same thing to the subject date_of_birth.

Using a higher stacklevel would have saved me a lot of time figuring this out, because it would have shown me a line in the traceback related to the field that was being parsed.

Checklist

  • Did you update CHANGELOG.md with your changes?
  • Have you checked our Contributing document?
  • Have you ensured the PR clearly describes the problem and the solution?
  • Is your contribution compliant with our coding style? This can be checked running flake8 from the source directory.
  • Have you checked to ensure that there aren't other open Pull Requests for the same change?
  • Have you included the relevant issue number using "Fix #XXX" notation where XXX is the issue number? By including "Fix #XXX" you allow GitHub to close issue #XXX when the PR is merged.

@codecov
Copy link

codecov bot commented Feb 2, 2023

Codecov Report

Merging #1641 (c90ac00) into dev (494e288) will increase coverage by 0.11%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev    #1641      +/-   ##
==========================================
+ Coverage   91.32%   91.43%   +0.11%     
==========================================
  Files          25       25              
  Lines        2535     2535              
  Branches      481      481              
==========================================
+ Hits         2315     2318       +3     
+ Misses        139      137       -2     
+ Partials       81       80       -1     
Flag Coverage Δ
integration 70.49% <0.00%> (ø)
unit 84.49% <100.00%> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/pynwb/file.py 88.77% <100.00%> (+0.74%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

rly
rly previously approved these changes Feb 2, 2023
@rly
Copy link
Contributor

rly commented Feb 2, 2023

Although it is not that relevant to this PR, could you please add a test for _add_missing_timezone in this or another PR? There is apparently no coverage of that line.

@rly
Copy link
Contributor

rly commented Feb 2, 2023

Otherwise, looks good to me. Thanks!

@rly rly mentioned this pull request Feb 15, 2023
6 tasks
@rly rly merged commit b7cdead into dev Feb 22, 2023
@rly rly deleted the add_stacklevel_to_missing_timezone branch February 22, 2023 23:11
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