Skip to content

rename grabbids to layout, closes #228 #230

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

Merged
merged 5 commits into from
Aug 10, 2018

Conversation

ltirrell
Copy link
Contributor

@ltirrell ltirrell commented Aug 9, 2018

Closes #228 by renaming bids/grabbids to bids/layout, but running creating a symlink from layout to grabbidsand keepinggrabbids in bids/__init__.py.

The README changes may not be the best, and no longer includes a pun.

@codecov-io
Copy link

codecov-io commented Aug 9, 2018

Codecov Report

Merging #230 into master will decrease coverage by 0.16%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #230      +/-   ##
==========================================
- Coverage   74.59%   74.42%   -0.17%     
==========================================
  Files          23       24       +1     
  Lines        2204     2209       +5     
  Branches      525      525              
==========================================
  Hits         1644     1644              
- Misses        409      414       +5     
  Partials      151      151
Flag Coverage Δ
#unittests 74.42% <50%> (-0.17%) ⬇️
Impacted Files Coverage Δ
bids/layout/bids_layout.py 59.6% <ø> (ø)
bids/reports/report.py 83.67% <ø> (ø) ⬆️
bids/reports/parsing.py 59.25% <ø> (ø) ⬆️
bids/version.py 0% <ø> (ø) ⬆️
bids/layout/bids_validator.py 91.02% <ø> (ø)
bids/variables/entities.py 87.64% <ø> (ø) ⬆️
bids/grabbids/__init__.py 0% <0%> (-100%) ⬇️
bids/layout/__init__.py 100% <100%> (ø)
bids/analysis/analysis.py 89.32% <100%> (ø) ⬆️

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 8fdf09a...d32fda4. Read the comment docs.

@ltirrell
Copy link
Contributor Author

ltirrell commented Aug 9, 2018

tests failed when symlinking grabbids to layout. now aliasing the module name loosely based of this. also seems cleaner (i would guess symlinks would break in windows at the very least).

also added a DeprecationWarning when importing bids.grabbids directly. not sure if that's necessary, or should have an improved message.

the tutorial ipynb now imports from bids.layout instead.

@tyarkoni
Copy link
Collaborator

tyarkoni commented Aug 9, 2018

Great, thanks! @chrisfilo, you okay with renaming the grabbids module to layout?

@chrisgorgo
Copy link
Contributor

Wouldn't this break a bunch of code doing from bids.grabbids import BIDSLayout or is the old syntax still supported?

@tyarkoni
Copy link
Collaborator

tyarkoni commented Aug 9, 2018

Old syntax is supported, with a deprecation warning, and we can eventually remove the old placeholder in a few releases.

__all__ = ["BIDSLayout", "BIDSValidator"]

warnings.simplefilter('always', DeprecationWarning)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd rather libraries not change the warnings registry. This makes it harder for programs to control their outputs.

__all__ = ["BIDSLayout", "BIDSValidator"]

warnings.simplefilter('always', DeprecationWarning)
warnings.warn("grabbids has been renamed to layout", DeprecationWarning)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we note in the warning text the version we're deprecating at (0.6.5 or 0.7), and when to expect its removal? This both helps downstream projects plan a transition path and lets pybids developers know when they can fully remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How's this for a proposed change to those lines:

-warnings.simplefilter('always', DeprecationWarning)
-warnings.warn("grabbids has been renamed to layout", DeprecationWarning)
+warnings.warn("grabbids has been renamed to layout in version 0.6.5, and will be removed in version 0.7", FutureWarning)

I could change the versions to whatever you think is best. FutureWarning shows up by default without changing the warning filter. I could also do that in another way

Copy link
Collaborator

Choose a reason for hiding this comment

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

@tyarkoni Will the next version be 0.6.5 or 0.7?

And I would say we should remove in 1.0, unless that's expected within the next month. (I realize pybids is alpha, but the grabbids API has been around for a long time.)

@chrisgorgo
Copy link
Contributor

chrisgorgo commented Aug 9, 2018 via email

@tyarkoni
Copy link
Collaborator

Thanks!

@tyarkoni tyarkoni merged commit 60a6a17 into bids-standard:master Aug 10, 2018
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.

5 participants