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

Fix PEP8 in data #929

Merged
merged 1 commit into from Mar 26, 2016

Conversation

Projects
None yet
3 participants
@ghoshbishakh
Member

ghoshbishakh commented Feb 18, 2016

fixes #874

@@ -631,8 +635,8 @@ def fetch_cenir_multib(with_raw=False):
'4e4324c676f5a97b3ded8bbb100bf6e5'])
files = {}
baseurl = \
'https://digital.lib.washington.edu/researchworks/bitstream/handle/1773/33311/'
baseurl = 'https://digital.lib.washington.edu/\

This comment has been minimized.

@samuelstjean

samuelstjean Feb 18, 2016

Contributor

no need to split url on two lines, even fi they go off at least they are easy to select and copy/paste.

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Feb 19, 2016

@samuelstjean I hope it is ok now

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Feb 20, 2016

if you close it without anyone merging it, it basicall means you don't want those change to be in the main dev version.

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Feb 20, 2016

@samuelstjean sorry for closing this but there was a test case failure arising and I am working on it afresh so that I can figure out which part is causing the problem. I planned to make a new PR. Is it a bad practice ? Then ill gladly reopen this one. 😄

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Feb 20, 2016

Well for really small thing it is not really worth it to close and make a new one, unless it has horrible merge problems and a tons of random commit got inside. If I do something big, I personnally prefer to break it in small cases (but you lose the point where it all fits in, and people don't bother checking the math) or you make a monster PR that is impossible to review.

@ghoshbishakh ghoshbishakh reopened this Feb 20, 2016

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Feb 20, 2016

so Ill fix the issue and squash the commits maybe and push in this PR then?

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Feb 20, 2016

@samuelstjean squashing and force pushing autometically closes a pull request as far as I remember. And I dont think so many commits for a small change are good. So can you suggest the workflow in this case?

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Feb 20, 2016

Well you only have 2 commits, I have seen worse. You can try a rebase to merge those ocmmits, like http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Feb 20, 2016

thanks a lot 😄
will update this PR asap

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Feb 20, 2016

No rush, it's not like I can merge it.

@@ -695,7 +698,8 @@ def read_cenir_multib(bvals=None):
Notes
-----
Details of the acquisition and processing, and additional meta-data are
available through `UW researchworks <https://digital.lib.washington.edu/researchworks/handle/1773/33311>`_
available through `UW researchworks
<https://digital.lib.washington.edu/researchworks/handle/1773/33311>`_

This comment has been minimized.

@arokem

arokem Mar 15, 2016

Member

I think this would break the rendering of the URL. You can't always have <80 characters...

@@ -297,7 +299,8 @@ def fetcher():
fetch_mni_template = _make_fetcher(
"fetch_mni_template",
pjoin(dipy_home, 'mni_template'),
'https://digital.lib.washington.edu/researchworks/bitstream/handle/1773/33312/',
'https://digital.lib.washington.edu/researchworks/\

This comment has been minimized.

@arokem

arokem Mar 15, 2016

Member

I'd rather leave this on one line.

@ghoshbishakh ghoshbishakh force-pushed the ghoshbishakh:pep8indata branch 2 times, most recently from e3f2e64 to 36f7f32 Mar 16, 2016

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Mar 16, 2016

@arokem I hope that solves the issue

@arokem

This comment has been minimized.

Member

arokem commented Mar 16, 2016

Yep. Looks good.

Note that you might have to rebase if we go forward with this and/or with #971

For the time being, I will start by merging this one in a couple of days, unless someone pipes up.

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Mar 16, 2016

@arokem sure no problem with git 😄

@arokem

This comment has been minimized.

Member

arokem commented Mar 23, 2016

This one needs a rebase.

@ghoshbishakh ghoshbishakh force-pushed the ghoshbishakh:pep8indata branch from 36f7f32 to af20cc5 Mar 26, 2016

@ghoshbishakh

This comment has been minimized.

Member

ghoshbishakh commented Mar 26, 2016

@arokem rebase done!

@arokem arokem merged commit 6bbf825 into nipy:master Mar 26, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment