search

bluesky / suitcase-tiff

http://nsls-ii.github.io/suitcase
Other
2 stars 5 forks source link

ENH: allows for formatting of the directory based on the start doc #23

Closed awalter-bnl closed 5 years ago

awalter-bnl commented 5 years ago

This would close issue #22 , as it adds the capability to template the directory path based on items from the start document.

Once it has been approved I will make similar changes to all the other suitcases.

awalter-bnl commented 5 years ago

@mrakitin I am planning on adding some more pytest fixutres to suitcase.utils to take care of that so they can be used for all suitcase repos. so that will likely be a seperate PR.

mrakitin commented 5 years ago

@mrakitin I am planning on adding some more pytest fixutres to suitcase.utils to take care of that so they can be used for all suitcase repos. so that will likely be a seperate PR.

Sure, sounds good :)

awalter-bnl commented 5 years ago

The tests from the latest push will fail until the PR below to suitcase-utils is merged https://github.com/NSLS-II/suitcase-utils/pull/17. but this should take care of @mrakitin's request above.

mrakitin commented 5 years ago

Recycled the PR to trigger a Travis build to pick up the merged changes from https://github.com/NSLS-II/suitcase-utils/pull/17.

mrakitin commented 5 years ago

Hmm, the tests still fail. I think we will have to tag suitcase-utils and push to PyPI, so that it can be installed from there.

mrakitin commented 5 years ago

@danielballan is the only maintainer at https://pypi.org/project/suitcase-utils. We have to wait until he returns, or install the deps from master. @awalter-bnl, what do you prefer?

awalter-bnl commented 5 years ago

cycling to restart the tests as the suitcase-utils PR it relied on has been merged

awalter-bnl commented 5 years ago

Just now noticed @mrakitin already tried to get the tests to pass. I am happy to wait on having this merged

awalter-bnl commented 5 years ago

I don’t think it is actually needed.

@danielballan there is a very clear use-case for this requested by sst. Can we leave this open until I am back from vacation and can discuss in more detail. Your comments do not actually cover the use case he has.

awalter-bnl commented 5 years ago

OK so after some more investigation I am 100% convinced that @danielballan is correct. we should update the docs to indicate (and include an example) that file_prefix can include path information as well as just a prefix for the file_name. This resolves the issue.

Note, please keep this open until I am back or someone has a chance to go over the changes in detail. As I added the file_prefix tests to suitcase-utils I also made several changes to suitcase-tiff.tiff_stack to get these tests to pass which will still need to be incorporated, either in this PR or in another.

danielballan commented 5 years ago

Will do, @awalter-bnl. Thanks.