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

Make alabaster and sphinx_rtd_theme dependencies optional #2097

Merged
merged 1 commit into from
Jan 19, 2016

Conversation

mitya57
Copy link
Contributor

@mitya57 mitya57 commented Oct 24, 2015

There is no need to pull these dependencies if i.e. the documentation is using the classic theme.

There will be no change in behavior for projects using alabaster or sphinx_rtd_theme.

Note: this PR does not remove these dependencies from setuptools requires list, as this can potentially break some reverse dependencies. If you think we should do this, please let me know and I will amend the PR. This change will be useful for us (Debian) anyway as we will be able to remove sphinx-rtd-theme from our our list of dependencies.

@keimlink
Copy link
Contributor

Seems reasonable! 👍

@birkenfeld
Copy link
Member

At least alabaster cannot be removed since it is the new default. The rtd theme was supposed to be merged, but the rtd guys wanted to have a little more time to do some changes, so we used the dependency solution to give them more flexibility.

Maybe it is time to revisit these choices for 1.4.

@mitya57
Copy link
Contributor Author

mitya57 commented Oct 27, 2015

@birkenfeld This PR does not change anything functionally, it just moves imports to where they are actually used.

As I said, I will be happy if at least sphinx-rtd-theme was removed from requirements (or merged), but this is not included in this pull request.

@mitya57
Copy link
Contributor Author

mitya57 commented Oct 29, 2015

Maybe it is time to revisit these choices for 1.4.

I am happy to retarget this PR to default branch, in case you do not want it in 1.3 (though, as I said, it doesn't break any existing setups).

@vitaut
Copy link
Contributor

vitaut commented Nov 15, 2015

👍 but I think it's even more important to remove dependencies from requires in setup.py. I did this in #2087 but only for sphinx-rtd-theme. The same could be done for alabaster.

@mitya57
Copy link
Contributor Author

mitya57 commented Nov 16, 2015

@vitaut Your PR is what I would like to be done, but mine is absolutely safe (i.e. it doesn't break anything), while yours can potentially introduce incompatibilities.

shimizukawa added a commit that referenced this pull request Jan 15, 2016
@shimizukawa
Copy link
Member

I've read this thread again, and +1 for this PR to merge into stable. If no objection, I'm going to merge into stable for sphinx-1.3.5.

@tk0miya
Copy link
Member

tk0miya commented Jan 16, 2016

@shimizukawa please tell me the plan of these dependencies before merging.
I hear the alabaster is default theme, and sphinx_rtd_theme will be removed at 1.4 (by #2087).
Is any reason to merge this? If you mean lazy loading of these themes, I agree.
Or not, I think we have to discuss more about the plan.

(I understand this is no breaking change, but I do not understand where we goes)

@shimizukawa
Copy link
Member

@tk0miya

Is any reason to merge this? If you mean lazy loading of these themes, I agree.
Or not, I think we have to discuss more about the plan.

The former one.

@tk0miya
Copy link
Member

tk0miya commented Jan 17, 2016

@shimizukawa I understand. LGTM!

@shimizukawa shimizukawa modified the milestones: 1.3.5, 1.4 Jan 18, 2016
There is no need to pull these dependencies if i.e. the documentation is
using the classic theme.

There will be no change in behavior for projects using alabaster or
sphinx_rtd_theme.
@mitya57
Copy link
Contributor Author

mitya57 commented Jan 19, 2016

FWIW I've rebased this pull request against the latest stable branch.

shimizukawa added a commit that referenced this pull request Jan 19, 2016
Make alabaster and sphinx_rtd_theme dependencies optional
@shimizukawa shimizukawa merged commit dcdcdb4 into sphinx-doc:stable Jan 19, 2016
@shimizukawa
Copy link
Member

Merged. Thanks!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2021
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.

6 participants