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

RF: move script running code into own module #418

Merged
merged 3 commits into from Oct 7, 2014

Conversation

Projects
None yet
2 participants
@matthew-brett
Member

matthew-brett commented Sep 13, 2014

Refactor code to run scripts into own module. In fact I'm using a version I
refactored for another project, to make it easier to use across projects, but
I think this makes the code easier to read and support.

RF: move script running code into own module
Refactor code to run scripts into own module.  In fact I'm using a
version I refactored for another project, to make it easier to use
across projects, but I think this makes the code easier to read and
support.
@arokem

This comment has been minimized.

arokem commented on dipy/tests/scriptrunner.py in b01a803 Sep 19, 2014

Do you mean above_us?

This comment has been minimized.

Owner

matthew-brett replied Sep 19, 2014

Dunno - I would say below, but is there some agreement about up and down in directory trees?

This comment has been minimized.

arokem replied Sep 19, 2014

I would say that .. is 'up'. The question is, who is 'us' here. I assume you are referring to the package_path?

@arokem

This comment has been minimized.

arokem commented on dipy/tests/scriptrunner.py in b01a803 Sep 19, 2014

For some reason package seems to be unset in python 3

This comment has been minimized.

This comment has been minimized.

Owner

matthew-brett replied Sep 19, 2014

Yup - this is the Python 3.2 issue I mentioned on the mailing list - I suggested dropping Python 3.2 support - what do you think?

This comment has been minimized.

arokem replied Sep 19, 2014

This comment has been minimized.

Owner

matthew-brett replied Sep 19, 2014

Aha - good question - we'd have to switch to a different mode for travis installs - using the Python virtualenvs. I have already worked that out for nipy, so I can submit a pull-request for that...

This comment has been minimized.

arokem replied Sep 19, 2014

@arokem

This comment has been minimized.

arokem commented on dipy/tests/scriptrunner.py in b01a803 Sep 19, 2014

Might want to shorten here by just having one if os.name == 'nt' statement here? The logic of that elif on line 119 is a tad confusing.

This comment has been minimized.

Owner

matthew-brett replied Sep 19, 2014

You mean add

if os.name == 'nt':
      if self.local_script_dir is None
            # Need .bat file extension for windows
            cmd[0] += '.bat'

etc?

This comment has been minimized.

arokem replied Sep 19, 2014

Reading through this again, I now realize that you would have to repeat one or the other (a test for local_script_dir or a test for os.name=='nt') so may as well leave it as it is.

@arokem

This comment has been minimized.

arokem commented on dipy/tests/scriptrunner.py in b01a803 Sep 19, 2014

typo => debug_print_v_a_r

@arokem

This comment has been minimized.

arokem commented on dipy/tests/test_scripts.py in b01a803 Sep 19, 2014

I must be missing something about the logic here. Why do you want this to be set by an environment variable?

This comment has been minimized.

Owner

matthew-brett replied Sep 19, 2014

Just so that scripts running the tests can turn on debug printing...

matthew-brett added some commits Sep 19, 2014

BF: workaround missing __package__ in Python 3.2
Python 3.2 does not set __package__; work around for now.
RF: change "below" to "above" for higher directory
I tend to think of directories nearer root as being 'down', it looks
more common to think of this as being 'up', change according to Ariel's
suggestion.
@arokem

This comment has been minimized.

Member

arokem commented Sep 25, 2014

Cool. Did you decide against the Travis reorg? For future reference, is this the relevant commit on nipy?

nipy/nipy@53e0fdf

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Oct 1, 2014

I thought I would work round the problem for now.

Here's my WIP for moving to the more standard build system: #428

@arokem

This comment has been minimized.

Member

arokem commented Oct 1, 2014

Cool. How about we hold off on this, until we resolve the Travis stuff?
Hopefully fast - I will take a look now.

On Wed, Oct 1, 2014 at 9:09 AM, Matthew Brett notifications@github.com
wrote:

I thought I would work round the problem for now.

Here's my WIP for moving to the more standard build system: #428
#428


Reply to this email directly or view it on GitHub
#418 (comment).

@arokem

This comment has been minimized.

Member

arokem commented Oct 6, 2014

OK - seems like we will need this anyway, considering that we might keep supporting 3.2, so I am going to go ahead and merge this.

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Oct 7, 2014

Thanks for the review...

On 10/6/14, Ariel Rokem notifications@github.com wrote:

OK - seems like we will need this anyway, considering that we might keep
supporting 3.2, so I am going to go ahead and merge this.


Reply to this email directly or view it on GitHub:
#418 (comment)

arokem added a commit that referenced this pull request Oct 7, 2014

Merge pull request #418 from matthew-brett/use-scriptrunner
RF: move script running code into own module

@arokem arokem merged commit e6daa20 into nipy:master Oct 7, 2014

1 check passed

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