libdynd / dynd-python

Python exposure of dynd
http://libdynd.org
Other
119 stars 23 forks source link

Refactor test discovery and add support for "python -m dynd.ndt.test". #699

Closed skrah closed 8 years ago

skrah commented 8 years ago

This adds support for runnings the tests as python -m dynd.ndt.test and python -m dynd.nd.test.

skrah commented 8 years ago

GitHub is still a bit weird. The pull request says:

"skrah wants to merge 1 commit into libdynd:master from skrah:split_tests2"

Shouldn't that be dynd-python:master?

insertinterestingnamehere commented 8 years ago

Hmm. It looks like it's listing the branches as user::branch. libdynd shows up because it's the name of the organization. I'm not sure if that's an error, a change in user interface, or something I just hadn't noticed before.

skrah commented 8 years ago

Looking at closed issues, it has always been libdynd:master. I think the GitHub outage has made me extra cautious today. :)

insertinterestingnamehere commented 8 years ago

Yah, that's pretty understandable.

skrah commented 8 years ago

Actually this isn't just refactoring, removing stateful things like __file__ from __init__.py is required for the moderately-magic version of the namespace package.

insertinterestingnamehere commented 8 years ago

Here, I've restarted the appveyor build to see if the error is just a fluke.

insertinterestingnamehere commented 8 years ago

Tests pass now. It'd be nice if we could get rid of the garbage commit though. Useful trick: if you amend the current commit without any changes and then force-push that, you can trigger a new build for a PR without having to throw in erroneous changes. That or we can restart the build for you.

insertinterestingnamehere commented 8 years ago

Since the other PR depends on this one, I can take care of some of the git oddities (or walk you through them) to get these merged if that'd help.

skrah commented 8 years ago

Thanks, I didn't know that trick to restart a build. In this case it's perhaps easiest to close this issue and just apply the namespace_package PR. All these commits are included there, too.

The garbage commit gets overwritten in that PR, the end result is exactly the same as rebasing/removing it.

While we're at it: I always thought that rebasing work that has already been published is discouraged. So that rule does not apply to pull requests?

izaid commented 8 years ago

This LGTM, thanks @skrah! I'll put it on the list of things to merge.

insertinterestingnamehere commented 8 years ago

Yep, that approach sounds great to me. We can just merge the other one. I've never had any trouble with rebasing PRs. It's common practice in a lot of other projects (e.g. numpy and scipy). The main issue with rebasing is that people who have pulled from the branch are no longer able to fast-forward to the newest version. With a PR that's very rarely a concern.

mwiebe commented 8 years ago

@skrah I don't think branches for PRs count as "published", that's why it's ok to rebase them.