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

change from using fparse to using parse #654

Merged
merged 2 commits into from
Nov 25, 2023
Merged

change from using fparse to using parse #654

merged 2 commits into from
Nov 25, 2023

Conversation

bendichter
Copy link
Contributor

@bendichter bendichter commented Nov 25, 2023

Over the last week or so I worked with the new dev of the parse library to incorporate the datetime parsing into that library: r1chardj0n3s/parse#165. This was the only feature that motivated the splinter of fparse, so we can now move our dependence back to parse with version >=1.20.0 and archive fparse.

There is one small difference in the parse implementation, which is that it infers whether the parsed value should be a datetime, a date, or a time. If no time information is present, parse returns a date object. NeuroConv expects a datetime, and I think PyNWB requires a datetime for the session_start_time, so for backwards compatibility and for better compatibility with PyNWB, I included a line that transforms dates to datetimes when extracting metadata.

I'd really like NWB to accept just a date value there, since the actual time is often missing and must be filled in with incorrect data. If NeurodataWithoutBorders/nwb-schema#542 ever gets merged we can switch this back to returning dates when only date information is given. However I don't want that issue to hold up this one so I'd like to move forward with datetimes for now.

Copy link

codecov bot commented Nov 25, 2023

Codecov Report

Merging #654 (e91a579) into main (1d3d58d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #654   +/-   ##
=======================================
  Coverage   91.61%   91.61%           
=======================================
  Files         107      107           
  Lines        5627     5630    +3     
=======================================
+ Hits         5155     5158    +3     
  Misses        472      472           
Flag Coverage Δ
unittests 91.61% <100.00%> (+<0.01%) ⬆️

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

Files Coverage Δ
src/neuroconv/tools/path_expansion.py 97.56% <100.00%> (+0.19%) ⬆️

@CodyCBakerPhD CodyCBakerPhD merged commit 8a793dc into main Nov 25, 2023
33 of 34 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the use_parse branch November 25, 2023 18:59
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.

2 participants