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

Remove unused validations from v1 config #4883

Merged
merged 6 commits into from
Jan 21, 2019

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Nov 9, 2018

Close #4706

I'm labeling this as wip because we still can delete more code from tests, but it will conflict with #4800, so waiting to get that one merged first

@stsewd stsewd added the PR: work in progress Pull request is not ready for full review label Nov 9, 2018
@codecov
Copy link

codecov bot commented Nov 9, 2018

Codecov Report

Merging #4883 into master will decrease coverage by 0.23%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #4883      +/-   ##
=========================================
- Coverage   76.74%   76.5%   -0.24%     
=========================================
  Files         158     158              
  Lines        9951   10004      +53     
  Branches     1244    1264      +20     
=========================================
+ Hits         7637    7654      +17     
- Misses       1980    2008      +28     
- Partials      334     342       +8
Impacted Files Coverage Δ
readthedocs/doc_builder/config.py 91.89% <ø> (ø) ⬆️
readthedocs/config/config.py 98.51% <ø> (-0.12%) ⬇️
readthedocs/vcs_support/backends/hg.py 59.09% <0%> (-5.43%) ⬇️
readthedocs/projects/utils.py 54.54% <0%> (-4.55%) ⬇️
readthedocs/restapi/views/integrations.py 88.65% <0%> (-3.12%) ⬇️
readthedocs/vcs_support/base.py 88.67% <0%> (-1.89%) ⬇️
readthedocs/vcs_support/backends/svn.py 27.69% <0%> (-1.82%) ⬇️
readthedocs/profiles/views.py 86.44% <0%> (-1.7%) ⬇️
readthedocs/restapi/views/model_views.py 94.08% <0%> (-1.04%) ⬇️
readthedocs/restapi/client.py 87.09% <0%> (-0.79%) ⬇️
... and 18 more

@stsewd
Copy link
Member Author

stsewd commented Nov 9, 2018

Ok, this is ready for review, global coverage drop is expected because we are removing a lot of code here. Coverage is looking good https://codecov.io/gh/rtfd/readthedocs.org/pull/4883/src/readthedocs/config/config.py?before=readthedocs/config/config.py

@stsewd stsewd removed the PR: work in progress Pull request is not ready for full review label Nov 9, 2018
Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

I don't understand it 100% but looks good.

It seems that we are not using base and name in the same way in V2. I'm assuming that V1 now uses a shared code with V2 and these checks are still performed but in a different way. If my assumption is true, I'd say that this is 👍

@stsewd
Copy link
Member Author

stsewd commented Dec 12, 2018

It seems that we are not using base and name in the same way in V2.

We never used that in v1, we were giving some defaults (constant strings actually) from the rtd code, and we never used those values, in v2 we have the multiple installations thing that makes what base was supposed to do, and we don't use name in v1 or v2.

Here https://github.com/rtfd/readthedocs.org/pull/4883/files#diff-8b90ee75bb4ed7a0341eac84832b7ea7

@stsewd stsewd requested a review from a team January 7, 2019 17:18
Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

Nice! Feel free to merge when you consider ;)

@stsewd stsewd merged commit b0ba3ff into readthedocs:master Jan 21, 2019
@stsewd stsewd deleted the remove-unused-validations branch January 21, 2019 16:36
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.

2 participants