-
Notifications
You must be signed in to change notification settings - Fork 22
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
New path API #10
New path API #10
Conversation
Related to gabrielcnr#7
pytest-datadir does not embed tests into the package
pytest_datadir/plugin.py
Outdated
assert os.path.isdir(self.module_dir) or os.path.isdir(self.global_dir),\ | ||
'neither {} or {} are valid data directories'.format(self.global_dir, self.module_dir) | ||
@pytest.fixture | ||
def global_datadir(request): |
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 wasn't sure about the name of this fixture.
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.
Also it is not really "global" because it only looks for a "data" folder on the same directory as the test file requesting it.
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 agree there is absolutely nothing "global" about that thing. Do we need it?
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.
This was added in #1... I never used this functionality but it seems useful. What do you think? Maybe @mauriciosl can comment on it as well?
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 see the motivation behind it. Basically if you have many test_*.py
files using the same test resource file, without the "global" data
dir you would be forced to copy the same file across many test data directories, which obviously is not a good practice. I vote to keep it.
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.
Oh sorry, I meant about the name of the fixture.
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.
Yo guys, let's get back to work here. If you guys would like to have me as maintainer ;) I can finish the PR so we could release the 1.0.
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.
Sounds good. About shared_data
, I think we can call it shared_datadir
and call it a day for 1.0
. What do you guys think?
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.
👍
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.
@igortg you're more than welcome to contribute! I also agree with @nicoddemus to just wrap up the shared_datadir fixture and release as 1.0.
Change suggested by @nicoddemus: it's not really "global" because it only looks for a "data" folder on the same directory as the test file requesting it (PR gabrielcnr#10)
Thanks @igortg! This needs to be rebased on |
# Conflicts: # README.md # pytest_datadir/__init__.py # pytest_datadir/plugin.py # tests/test_hello.py
Older versions of setuptools seems not work with environment marker (i.e.: "python_version<"3.4")
Guys, all set. Waiting for review. |
LGTM, thanks @igortg! 👍 Let's wait for @gabrielcnr say on this. |
Thanks @gabrielcnr. If it is alright with you, I will add igor's user to PyPI so he can make the release. |
WIP towards changing the API to
pathlib
as discussed in #7Still needs docs and probably merge the other PRs before moving this forward.