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

DONT_INSTALL_DOCUTILS feature flag #7276

Merged
merged 4 commits into from Jul 23, 2020

Conversation

humitos
Copy link
Member

@humitos humitos commented Jul 8, 2020

Allow people to install latest release of docutils (currently, 0.16) not install docutils at all

Closes #7275

Allow people to install latest release of docutils (currently, 0.16)
@humitos humitos requested review from agjohnson and a team Jul 8, 2020
@stsewd
Copy link
Member

stsewd commented Jul 8, 2020

I think we can just remove that dependency, and leave it to sphinx to decide the version

https://github.com/sphinx-doc/sphinx/blob/cbc16eb384a0fc6181a4543c34977e794cae231d/setup.py#L26

@humitos
Copy link
Member Author

humitos commented Jul 8, 2020

That makes sense to me. I wonder why we had it originally. Maybe older versions of Sphinx does not have this requirement?

This requirement has been there "forever". This is the original commit where that file is created from projects/tasks.py: a6f2094

Yeah, it seems it was added to support old Sphinx versions: 37ea03a

@humitos
Copy link
Member Author

humitos commented Jul 8, 2020

Using a feature flag is the safest and immediate change to do 😄 --we can try removing the dependency with another feature flag to cover ourselves in a different PR, I suppose. Thoughts?

@stsewd
Copy link
Member

stsewd commented Jul 8, 2020

Looks like sphinx includes docutils since the beginning https://github.com/sphinx-doc/sphinx/blob/be6aa3940e2efc0a9372072ea4e6ae4525f0a2cc/setup.py#L47

So, I guess the problem was with a breaking change in docutils and incompatible with sphinx or something like that.

@agjohnson
Copy link
Contributor

agjohnson commented Jul 8, 2020

I'd be fine skipping to a feature flag that doesn't install docutils at all. It seems like it was likely a temporary concern anyways.

@humitos humitos changed the title USE_DOCUTILS_LATEST feature flag DONT_INSTALL_DOCUTILS feature flag Jul 9, 2020
@humitos
Copy link
Member Author

humitos commented Jul 9, 2020

I changed the meaning of the flag. Please, re-review his.

stsewd
stsewd approved these changes Jul 22, 2020
@humitos humitos merged commit 1d2e90c into master Jul 23, 2020
2 checks passed
@humitos humitos deleted the humitos/use-docutils-latest-feature-flag branch Jul 23, 2020
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.

Use up to date version of docutils by default
3 participants