Skip to content

RF: Add extension_initial_dot config option to transition to extension entity with initial dot#629

Merged
effigies merged 11 commits intobids-standard:masterfrom
effigies:rf/dot_extension_mode
Jun 29, 2020
Merged

RF: Add extension_initial_dot config option to transition to extension entity with initial dot#629
effigies merged 11 commits intobids-standard:masterfrom
effigies:rf/dot_extension_mode

Conversation

@effigies
Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator Author

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Couple quick notes. Awkward name, so happy to change it if there's a better one.

"this warning and include the leading dot, use "
"`bids.config.set_option('extension_initial_dot', True)`.",
FutureWarning)
config = "bids-nodot"
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is where the warning is emitted. It's called with every new layout.

if new_pattern != old_pattern:
warnings.warn(
"Cannot modify extension_initial_dot option after initialization. "
"Set option immediately after ``import bids``.")
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'm not entirely positive how to reach this...


run = dataset.create_node('run', entities, image_file=img_f,
duration=duration, repetition_time=tr)
run_info = run.get_info()
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Tal, I'm not sure how these changes hit this code path, but we were returning a RunNode sometimes in a function that is always supposed to return a NodeIndex. I'm assuming it's a refactoring artifact, but I haven't dug through the git-blame.

@codecov
Copy link
Copy Markdown

codecov bot commented Jun 23, 2020

Codecov Report

Merging #629 into master will increase coverage by 0.01%.
The diff coverage is 87.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #629      +/-   ##
==========================================
+ Coverage   83.91%   83.93%   +0.01%     
==========================================
  Files          26       26              
  Lines        3252     3274      +22     
  Branches      826      836      +10     
==========================================
+ Hits         2729     2748      +19     
- Misses        334      336       +2     
- Partials      189      190       +1     
Flag Coverage Δ
#unittests 83.93% <87.50%> (+0.01%) ⬆️
Impacted Files Coverage Δ
bids/reports/report.py 83.33% <ø> (ø)
bids/layout/models.py 89.05% <58.33%> (-1.54%) ⬇️
bids/config.py 81.39% <100.00%> (+1.39%) ⬆️
bids/layout/index.py 87.06% <100.00%> (+0.16%) ⬆️
bids/layout/layout.py 86.07% <100.00%> (+0.07%) ⬆️
bids/layout/writing.py 85.71% <100.00%> (+0.42%) ⬆️
bids/variables/io.py 78.71% <100.00%> (+0.99%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2b753c...67d62af. Read the comment docs.

@effigies effigies added this to the 0.11 milestone Jun 24, 2020
@effigies
Copy link
Copy Markdown
Collaborator Author

Bumping on this. It would be nice to finally release 0.11, and I figured we might as well have a planned breaking change started sooner than later.

@effigies
Copy link
Copy Markdown
Collaborator Author

Tests are passing and the only coverage missing is a hard (impoosible?) to reach edge case. I'm boing to go ahead with this and release. We can roll back in 0.11.1 if I've catastrophically miscalculated.

@effigies effigies merged commit f9426cc into bids-standard:master Jun 29, 2020
@effigies effigies deleted the rf/dot_extension_mode branch June 29, 2020 17:44
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.

Please return leading . in extension

1 participant