Skip to content

Removed un-needed python dependencies #5389

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

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 a team March 4, 2019 10:40
@@ -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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

We still need to keep come libs here

@Mike-Dai
Copy link
Contributor Author

Mike-Dai commented Mar 5, 2019

@stsewd Made the change. Please review.

@humitos
Copy link
Member

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 commented Mar 5, 2019

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

@stsewd stsewd merged commit b6d6528 into readthedocs:master Mar 5, 2019
@stsewd
Copy link
Member

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
Development

Successfully merging this pull request may close these issues.

4 participants