-
Notifications
You must be signed in to change notification settings - Fork 202
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
support path separators in cassette names #107
Conversation
|
||
} | ||
|
||
class TestStorage extends AbstractStorage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i hope its ok for you to have this class right in the test case file. imho it makes sense to keep them here, its "local" to just this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah its good, makes sense.
Yeah looks good. Are the tests working on your system? |
/** | ||
* Expect error | ||
*/ | ||
public function testFilePathCreated() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo: how can we test for this? does vfsStream provide anything?
okay, i am green locally. |
throw new VCRException(sprintf('Cassette path "%s" is not existing or not a directory', $cassettePath), 0); | ||
} | ||
|
||
$file = rtrim($cassettePath, DIRECTORY_SEPARATOR) . DIRECTORY_SEPARATOR . $cassetteName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels a bit like the constructor is doing too much. I would probably extend it to a method/helper. But its right "at the edge" so you might as well leave it like it is.
Its just the Assertion, otherwise I think its good. Thank you for this, cool feature. |
okay, updated to use the assertion. i had a look around and tried to see where i would document this but i can't find the right place to add this. it seems the whole cassette concept is never explained, only used in the examples. i suggest adding a section called "Usage" between "Installation" and "Configuration", and move the phpunit annotation that is in "Installation" to usage as well. then the usage can explain the main factory calls some more. shall i create an issue asking for that in the doc-repository? not sure if i find time to do that soon |
support path separators in cassette names
Documentation is always nice :-) An issue would be good, but to be honest I don't know if I can get to it either. Thank you for this nice little feature! |
*/ | ||
public function __construct($cassettePath, $cassetteName, $defaultContent = '[]') | ||
{ | ||
$file = $cassettePath . DIRECTORY_SEPARATOR . $cassetteName; | ||
Assertion::directory($cassettePath, 'Cassette path "{$cassettePath}" is not existing or not a directory'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah wait I just saw the wrong usage of quotes here. I'll fix it on master
created php-vcr/php-vcr.github.io#6 about the doc |
This change makes it possible to group fixtures by folder names.
To work correctly on windows systems, you need to use PATH_SEPARATOR rather than hardcode
/
.