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 trailing slashes on svn checkout #4951

Merged
merged 8 commits into from Dec 26, 2018

Conversation

@dojutsu-user
Copy link
Member

@dojutsu-user dojutsu-user commented Dec 4, 2018

Fixes #4934

@codecov
Copy link

@codecov codecov bot commented Dec 4, 2018

Codecov Report

Merging #4951 into master will decrease coverage by 0.09%.
The diff coverage is 83.33%.

@@            Coverage Diff            @@
##           master    #4951     +/-   ##
=========================================
- Coverage   76.93%   76.84%   -0.1%     
=========================================
  Files         158      158             
  Lines       10039     9942     -97     
  Branches     1259     1241     -18     
=========================================
- Hits         7724     7640     -84     
+ Misses       1981     1969     -12     
+ Partials      334      333      -1
Impacted Files Coverage Δ
readthedocs/vcs_support/backends/svn.py 39.39% <83.33%> (+9.88%) ⬆️
readthedocs/projects/version_handling.py 91.83% <0%> (-2.76%) ⬇️
readthedocs/projects/views/private.py 80.05% <0%> (-0.06%) ⬇️
readthedocs/projects/constants.py 100% <0%> (ø) ⬆️
readthedocs/projects/forms.py 79.66% <0%> (ø) ⬆️
readthedocs/restapi/serializers.py 97.43% <0%> (ø) ⬆️
readthedocs/projects/models.py 85.55% <0%> (+0.11%) ⬆️
readthedocs/vcs_support/backends/git.py 82.27% <0%> (+0.11%) ⬆️
readthedocs/builds/models.py 78.83% <0%> (+0.41%) ⬆️
readthedocs/doc_builder/python_environments.py 83.44% <0%> (+0.46%) ⬆️
... and 2 more

Loading

@stsewd
Copy link
Member

@stsewd stsewd commented Dec 4, 2018

I'm not so familiar with svn, but I guess we could do this a little more robust. Could be the case where the branch/tag doesn't have a /.

Loading

@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Dec 4, 2018

@stsewd
I have implemented the lstrip to deal with case in your comment above.

Loading

readthedocs/vcs_support/backends/svn.py Outdated Show resolved Hide resolved
Loading
Copy link
Member

@stsewd stsewd left a comment

LGTM, but would be great to have some tests here, we don't have tests for svn right now because that would require us to install svn on travis/locally, not sure. So, what about jus putting this on its own function/method and test only that?

Loading

@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Dec 5, 2018

@stsewd
okay... that would be great.
One thing, since there are no tests for svn as of now, so where should you thing the tests should be?
I am thinking to create a new test file named - test_backend_svn.py ?

Loading

@stsewd
Copy link
Member

@stsewd stsewd commented Dec 5, 2018

yeah, that sounds great

Loading

@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Dec 5, 2018

@stsewd
I just noticed that there is already a file test_backend.py which contains the tests for Git and Mercurial.
I think the test can be added there only.

    readthedocs.org/readthedocs/rtd_tests/tests/test_backend.py

Loading

@stsewd
Copy link
Member

@stsewd stsewd commented Dec 5, 2018

Actually, I wanted to start to separate the test files, some are very big already, not sure anyway.

Loading

@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Dec 5, 2018

@stsewd
Okay

Loading

stsewd
stsewd approved these changes Dec 5, 2018
Copy link
Member

@stsewd stsewd left a comment

Thanks!

Loading

@ericholscher ericholscher merged commit 70606a9 into readthedocs:master Dec 26, 2018
3 checks passed
Loading
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

3 participants