Skip to content
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

Add suffix to doc context. Fixes #1873. #1876

Merged
merged 1 commit into from Jun 7, 2015

Conversation

ericholscher
Copy link
Contributor

Not sure the best way to get the suffix from the docname, but this seems to work well. Happy to clean this up if there's a better way to do it.

Refs #1873

@ericholscher
Copy link
Contributor Author

Any thoughts?

@RobRuana
Copy link
Contributor

RobRuana commented Jun 1, 2015

This could use at least one test to verify it is working correctly.

Also, are we sure that there will always be a suffix? If not you'd end up with the whole path in suffix. Maybe something like:

docpath = self.env.doc2path(docname)
suffix = ""
if "." in docpath:
    suffix = docpath.split('.')[-1]

@shimizukawa
Copy link
Member

LGTM.

self.env.doc2path(docname) always return a path with suffix.
If worry about it, we can use os.path.splitext(self.env.doc2path(docname))[-1] more safely.

@ericholscher
Copy link
Contributor Author

Yea, is there a specific test that I should be adding this to? I looked through, but didn't find a great place to put it, but it's likely I missed it.

@shimizukawa
Copy link
Member

If I write a test, it will be written to tests/test_build_html.py.
However, there is no existing tests to check the context.
You can either newly prepared, or not write the test :/

@ericholscher
Copy link
Contributor Author

Yea, that sounds like a good amount of work. If there was something that was already introspecting the inner build state, I would happily add another, but I don't have time currently to build a full test around it.

@shimizukawa
Copy link
Member

OK, I'm going to merge this.
Before that, @ericholscher please can you squash the commits into 1 commit?

@ericholscher
Copy link
Contributor Author

Squashed commits.

shimizukawa added a commit that referenced this pull request Jun 7, 2015
@shimizukawa shimizukawa merged commit 381d9ef into sphinx-doc:master Jun 7, 2015
@shimizukawa
Copy link
Member

Merged. Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants