Skip to content

Clarify PYTHONPATH changes and rootdir roles#2595

Merged
The-Compiler merged 1 commit into
pytest-dev:masterfrom
nicoddemus:docs-rootdir-pythonpath
Jul 24, 2017
Merged

Clarify PYTHONPATH changes and rootdir roles#2595
The-Compiler merged 1 commit into
pytest-dev:masterfrom
nicoddemus:docs-rootdir-pythonpath

Conversation

@nicoddemus

Copy link
Copy Markdown
Member
  • Also minor adjustments in the docs (wording, formatting, links, etc).

Fix #2589

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.0009%) to 92.122% when pulling 5ee55a0 on nicoddemus:docs-rootdir-pythonpath into c92760d on pytest-dev:master.

Comment thread doc/en/customize.rst Outdated
Here's a summary what ``pytest`` uses ``rootdir`` for:

* Constructing ``nodeids`` during collection; each test is assigned
a unique ``nodeid`` which is rooted at the ``rootdir`` and takes in account full path,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nitpick: Why would nodeid and ini-file (both singular/plural) be monospaced? Those are "concepts", not variable names or code, right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

You are right, consistency is important (not nitpick at all IMHO) 😉

Comment thread doc/en/example/index.rst Outdated
.. _examples:

Usages and Examples
Examples and customizations tricks

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nitpick: customization (singular)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

👍

Comment thread doc/en/pythonpath.rst Outdated
root/
|- foo/
|- __init__.py
|- contest.py

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

contest -> conftest?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed

Comment thread doc/en/pythonpath.rst Outdated

root/
|- foo/
|- contest.py

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

contest -> conftest?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed

@The-Compiler

Copy link
Copy Markdown
Member

Also cc @pfctdayelise @hackebrot who probably have some more experience with writing/reviewing documentation than I do.

- Also minor adjustments in the docs (wording, formatting, links, etc).

Fix pytest-dev#2589
@nicoddemus nicoddemus force-pushed the docs-rootdir-pythonpath branch from 5ee55a0 to 3d24485 Compare July 21, 2017 10:31
@nicoddemus

Copy link
Copy Markdown
Member Author

Done all changes requested by @The-Compiler, thanks!

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.0009%) to 92.092% when pulling 3d24485 on nicoddemus:docs-rootdir-pythonpath into ccc4b3a on pytest-dev:master.

@nicoddemus

Copy link
Copy Markdown
Member Author

@The-Compiler anything else you would like me to change? If not, could you update the status to approve? 😉

Gentle ping @hackebrot @pfctdayelise

@The-Compiler

Copy link
Copy Markdown
Member

I think this is clearly an improvement over the status quo, so let's merge it 😉 Thanks!

@The-Compiler The-Compiler merged commit 81ad185 into pytest-dev:master Jul 24, 2017
@nicoddemus

Copy link
Copy Markdown
Member Author

Thanks!

@nicoddemus nicoddemus deleted the docs-rootdir-pythonpath branch July 24, 2017 13:07
@hackebrot

Copy link
Copy Markdown
Member

Hey @nicoddemus @The-Compiler! 👋

Sorry for the late response. I started my review right when you merged this PR. 😁 I'll submit a new PR with any changes that I was about to suggest, so we can discuss there. 👍

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.

4 participants