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

Removed un-needed python dependencies #5389

Merged
merged 4 commits into from Mar 5, 2019
Merged

Conversation

@Mike-Dai
Copy link
Contributor

@Mike-Dai Mike-Dai commented Mar 4, 2019

Fixes: #5101
Checked pip.txt and removed un-needed python dependencies.

@humitos humitos requested a review from Mar 4, 2019
@@ -22,8 +22,6 @@ jsonfield==2.0.2
requests==2.21.0
requests-toolbelt==0.9.1
slumber==0.7.1
lxml==4.3.1
defusedxml==0.5.0
Copy link
Member

@stsewd stsewd Mar 4, 2019

I think those are needed when building without using the docker images. We should test that people building without docker can still use their local instance before merging.

Copy link
Member

@stsewd stsewd Mar 4, 2019

Looks like we still need these packages #3082
At least before #5293 is merged

Copy link
Member

@humitos humitos Mar 5, 2019

We shouldn't care about people building without Docker. It's unsupported.

Copy link
Member

@ericholscher ericholscher Mar 5, 2019

Have we ever officially documented that we don't support this? I think it's useful for development and much easier for users when they are getting started. Making users install and download docker images adds a huge dependency to people trying to get started.

Copy link
Member

@stsewd stsewd Mar 5, 2019

We only have discussed this, but I agree, It would make harder to start contributing.

Anyway, these libs were needed for api v1.

Copy link
Member

@humitos humitos Mar 5, 2019

We didn't document this, but we talked in different opportunities. Most (or all?) of the build changes are tested under the process that uses Docker, so we can't guarantee that local building produces the same output than building with Docker.

Copy link
Member

@stsewd stsewd left a comment

We still need to keep come libs here

@Mike-Dai
Copy link
Contributor Author

@Mike-Dai Mike-Dai commented Mar 5, 2019

@stsewd Made the change. Please review.

@humitos
Copy link
Member

@humitos humitos commented Mar 5, 2019

@Mike-Dai considering that we are removing APIv1, please put the XML requirement together and add a comment above it saying that this has to be removed once v1 is removed (linking the PR that removes it and the PR that adds them as dependencies)

@Mike-Dai
Copy link
Contributor Author

@Mike-Dai Mike-Dai commented Mar 5, 2019

@humitos
These two links are added :
#5293
#3082
Please review.

humitos
humitos approved these changes Mar 5, 2019
stsewd
stsewd approved these changes Mar 5, 2019
@stsewd stsewd merged commit b6d6528 into readthedocs:master Mar 5, 2019
1 check passed
@stsewd
Copy link
Member

@stsewd stsewd commented Mar 5, 2019

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants