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

Implement cmd as List #168

Merged
merged 10 commits into from Nov 14, 2019

Conversation

@wesbarnett
Copy link
Contributor

wesbarnett commented Nov 8, 2019

Submission Checklist

  • Run unit tests
  • Declare copyright holder and open-source license: see below

Summary

Changes command into a List. Previously it was a String and .split() was used. In addition an absolute path is used for the target stanfile in the scenario where it is copied from a tmpdir.

Fixes #167 and fixes #91.

Copyright and Licensing

Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company): American Express

By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses:

Copy link
Member

mitzimorris left a comment

looks good - could you add a unit test for a pathname that contains a space?

@wesbarnett

This comment has been minimized.

Copy link
Contributor Author

wesbarnett commented Nov 9, 2019

Sure, I can add that. Would you prefer that in the same PR or a separate one?

@mitzimorris

This comment has been minimized.

Copy link
Member

mitzimorris commented Nov 9, 2019

same PR, please

Copy link
Member

mitzimorris left a comment

beautiful! many thanks!

@@ -245,6 +245,80 @@ def test_fixed_param_good(self):
self.assertEqual(datagen_fit.metric_type, None)
self.assertEqual(datagen_fit.stepsize, None)

def test_bernoulli_file_with_space(self):
stan = os.path.join(datafiles_path, 'bernoulli with space in name.stan')

This comment has been minimized.

Copy link
@ahartikainen

ahartikainen Nov 11, 2019

Contributor

Is there a difference with filename with space and filepath with space?

This comment has been minimized.

Copy link
@wesbarnett

wesbarnett Nov 11, 2019

Author Contributor

Interestingly, there might be. I get different error codes without the patch for the case of a file with spaces vs a directory with spaces. In both cases this PR fixes it. So, I can add another test for a path with spaces.

@wesbarnett

This comment has been minimized.

Copy link
Contributor Author

wesbarnett commented Nov 11, 2019

I'm essentially copying the good test case and modifying the filename/path for the unittest. Maybe there is a better approach with that so that the same test could be re-used with just changing the stan file name/path?

Refactor to just re-use good test with different stan file
@mitzimorris

This comment has been minimized.

Copy link
Member

mitzimorris commented Nov 11, 2019

Maybe there is a better approach with that so that the same test could be re-used with just changing the stan file name/path?

http://doc.pytest.org/en/latest/reference.html#pytest-mark-parametrize-ref

parameterize a function - call it several times with path/filenames?

@wesbarnett

This comment has been minimized.

Copy link
Contributor Author

wesbarnett commented Nov 11, 2019

Maybe there is a better approach with that so that the same test could be re-used with just changing the stan file name/path?

http://doc.pytest.org/en/latest/reference.html#pytest-mark-parametrize-ref

parameterize a function - call it several times with path/filenames?

Perfect. Will take a look.

@wesbarnett

This comment has been minimized.

Copy link
Contributor Author

wesbarnett commented Nov 11, 2019

It looks like pytest's parametrize doesn't work with unittest.TestCase: https://docs.pytest.org/en/latest/unittest.html

This reverts commit 7340629.
@wesbarnett

This comment has been minimized.

Copy link
Contributor Author

wesbarnett commented Nov 11, 2019

Since pytest's parametrize doesn't work with unittest.TestCase, I made the name of the stanfile an argument to the test and created two new tests with two different filenames. The relevant changes are here.

@mitzimorris mitzimorris merged commit bf477e1 into stan-dev:master Nov 14, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@wesbarnett wesbarnett deleted the wesbarnett:cmdarray branch Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.