Skip to content

tests: cover absolute path handling in _compute_fixture_value #6604

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

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Jan 28, 2020

No description provided.

@blueyed blueyed force-pushed the tests-_compute_fixture_value-cover-abs-source_path_str branch from 2defdce to 1cf9e68 Compare January 28, 2020 17:53
blueyed added a commit to blueyed/pytest that referenced this pull request Jan 28, 2020
Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Please rebase/add a descriptive commit message.

@blueyed
Copy link
Contributor Author

blueyed commented Jan 28, 2020

@nicoddemus

Please rebase/add a descriptive commit message.

Not sure what you mean.. do you mean it should be squashed?
I've done it in 2 commits, since the 2nd is a partial cherry-pick, and AFAICS the commit messages are fine?

@nicoddemus
Copy link
Member

Not sure what you mean.. do you mean it should be squashed?

Oops sorry, yes.

I've done it in 2 commits, since the 2nd is a partial cherry-pick, and AFAICS the commit messages are fine?

The commit messages don't really explain the context of the changes, for example the first one is just "source_path: py.path.local directly", which explains what it is doing, but not "why". I always try to think of the POV of someone who needs to look at the history a few years down the road and needs to figure out why things are they way they are.

But if they do look fine to you, please go ahead and merge. And thanks for the coverage improvement. 👍

@blueyed
Copy link
Contributor Author

blueyed commented Jan 28, 2020

"source_path: py.path.local directly" could be "Assign source_path via py.path.local directly", but should really be clear from the change itself (7c87874). The reference there is where it is taken from (in features), and keeping it separate helps with conflicts when rebasing/merging.
Thanks for the suggestion / review though.. 👍

@blueyed blueyed merged commit 2d29c3e into pytest-dev:master Jan 28, 2020
@blueyed blueyed deleted the tests-_compute_fixture_value-cover-abs-source_path_str branch January 28, 2020 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants