Skip to content
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

Rename top-level functions to not shadow Octave functions #42

Closed
apjanke opened this issue Mar 18, 2019 · 8 comments
Closed

Rename top-level functions to not shadow Octave functions #42

apjanke opened this issue Mar 18, 2019 · 8 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@apjanke
Copy link
Owner

apjanke commented Mar 18, 2019

We are currently providing test, runtests, and __run_test_suite__ functions that shadow Octave's own functions. Since Testify is turning into a longer-lived project, this is no longer appropriate. Rename them so they can coexist side by side with Octave's built-in test functions.

This is a high priority change, so it'll get its own minor release: 0.3.0.

@apjanke apjanke self-assigned this Mar 18, 2019
@apjanke apjanke added the bug Something isn't working label Mar 18, 2019
@apjanke apjanke added this to the 0.3.0 milestone Mar 18, 2019
@apjanke
Copy link
Owner Author

apjanke commented Mar 18, 2019

Done in 5defca4, but waiting to hear from collaborators about what they think of the new names.

@cbm755
Copy link
Contributor

cbm755 commented Mar 18, 2019

Don't care too much :)

__run_test_suite__ should go away, IMHO (or become a very thin layer around runtests2).

I think runtests2 and test2 should converge, to test2.

So how about testng for "next gen"?

@apjanke
Copy link
Owner Author

apjanke commented Mar 19, 2019

I'm not a big fan of "ng" or "new" for names, because they don't age well. If you later make another implementation, then the "new" one is actually an old one.

I think runtests2 and test2 should converge, to test2.

I'm not sure about this. They work at different levels: test2 runs tests in a single file, outputting detailed results for those tests. runtests2 discovers and runs tests for multiple files, with a coarser-grained aggregate results output. I'd rather deal with that using composition than a bunch of behavior switches in a single implementation.

Maybe they converge at the interface level, and just have separate implementation objects/functions for the two operational levels.

@cbm755
Copy link
Contributor

cbm755 commented Mar 19, 2019

Maybe they converge at the interface level, and just have separate implementation objects/functions for the two operational levels.

+1 User experience is "I want to test foo", where foo is function, dir, class, namespace, package, ...

@apjanke
Copy link
Owner Author

apjanke commented Mar 19, 2019

Cool.

One issue is that the existing test and runtests and their signatures are part of the public Octave API. We may have to take a breaking change on that if we want to merge them. I think if we merge them into test, the basic calling forms of test would still work; you'd just get additional calling forms. And we could leave a runtests wrapper in place for a while that just issues a deprecation warning and passes the call to test. But I'm not sure.

@apjanke
Copy link
Owner Author

apjanke commented Mar 19, 2019

Let's take the discussion on merging the functions over to #46. I'd like to close this issue out for Milestone 0.3.0, since this was initially just about avoiding the name collisions.

@apjanke
Copy link
Owner Author

apjanke commented Mar 19, 2019

Closing as fixed since the rename has landed.

@apjanke apjanke closed this as completed Mar 19, 2019
@apjanke
Copy link
Owner Author

apjanke commented Mar 19, 2019

I pushed this out in v0.3.0, just released now. https://github.com/apjanke/octave-testify/releases/tag/v0.3.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants