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

Refactor dataset configuration code as constructors #659

Merged
merged 5 commits into from
Nov 27, 2023

Conversation

h-mayorquin
Copy link
Collaborator

As discussed in #569

#569 (comment)

This should keep code that is related together and simplify code navigation.

@h-mayorquin h-mayorquin marked this pull request as ready for review November 26, 2023 13:04
@h-mayorquin h-mayorquin self-assigned this Nov 26, 2023
@CodyCBakerPhD
Copy link
Member

CodyCBakerPhD commented Nov 27, 2023

I think this pattern does indeed look much better, and is more standard/consistent with other class constructors in our ecosystem too

One small comment about the annotation typing for the return

Copy link

codecov bot commented Nov 27, 2023

Codecov Report

Merging #659 (a8ea15f) into main (badf452) will increase coverage by 0.00%.
Report is 2 commits behind head on main.
The diff coverage is 88.23%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #659   +/-   ##
=======================================
  Coverage   91.61%   91.62%           
=======================================
  Files         107      107           
  Lines        5630     5646   +16     
=======================================
+ Hits         5158     5173   +15     
- Misses        472      473    +1     
Flag Coverage Δ
unittests 91.62% <88.23%> (+<0.01%) ⬆️

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

Files Coverage Δ
...roconv/tools/nwb_helpers/_dataset_configuration.py 90.62% <77.77%> (+0.62%) ⬆️
...euroconv/tools/nwb_helpers/_models/_base_models.py 96.74% <90.47%> (-3.26%) ⬇️

@CodyCBakerPhD CodyCBakerPhD merged commit cf9e324 into main Nov 27, 2023
35 of 36 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the refactor_io_configuration_method_as_builders branch November 27, 2023 19:56
@CodyCBakerPhD
Copy link
Member

Interesting - automerge did not trigger when a job was manually cancelled (having some kind of intermittent stall problem with mac runners...) even after all conditions passed

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