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 project model fields #4870

Merged
merged 21 commits into from
Dec 4, 2018
Merged

Remove unused project model fields #4870

merged 21 commits into from
Dec 4, 2018

Conversation

dojutsu-user
Copy link
Member

Fixes #4866

@codecov
Copy link

codecov bot commented Nov 6, 2018

Codecov Report

Merging #4870 into master will decrease coverage by 0.14%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4870      +/-   ##
==========================================
- Coverage   76.93%   76.79%   -0.15%     
==========================================
  Files         158      158              
  Lines       10038     9932     -106     
  Branches     1259     1241      -18     
==========================================
- Hits         7723     7627      -96     
+ Misses       1981     1973       -8     
+ Partials      334      332       -2
Impacted Files Coverage Δ
readthedocs/builds/models.py 78.83% <ø> (+0.41%) ⬆️
readthedocs/projects/version_handling.py 91.83% <ø> (-2.76%) ⬇️
readthedocs/restapi/views/model_views.py 95.12% <ø> (+0.96%) ⬆️
readthedocs/projects/views/private.py 80.05% <ø> (-0.06%) ⬇️
readthedocs/restapi/serializers.py 97.43% <ø> (ø) ⬆️
readthedocs/projects/constants.py 100% <ø> (ø) ⬆️
readthedocs/projects/forms.py 79.66% <ø> (ø) ⬆️
readthedocs/projects/models.py 85.55% <100%> (+0.11%) ⬆️
readthedocs/projects/admin.py 91.5% <100%> (ø) ⬆️

@dojutsu-user
Copy link
Member Author

@humitos
Please review.

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.

LGTM! Thanks.

I'd wait for another one to take a look at this PR before merging because it has an impact in the database.

@humitos humitos requested a review from a team November 7, 2018 09:00
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.

@dojutsu-user
Copy link
Member Author

dojutsu-user commented Nov 7, 2018

@stsewd
Should we remove this test file?

    readthedocs.org/readthedocs/rtd_tests/tests/test_supported.py

I think, this is no longer needed

@stsewd
Copy link
Member

stsewd commented Nov 8, 2018

@dojutsu-user seems safe to remove

@dojutsu-user
Copy link
Member Author

@stsewd
There's a job error in Travis.
Is it related to this PR?

@stsewd
Copy link
Member

stsewd commented Nov 9, 2018

Just restarted the job. I'm not sure if this PR is ready for review, but there is more code to remove.

@dojutsu-user
Copy link
Member Author

@stsewd
Can you please review it again?

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.

This is a lot of code removed 🎉, I rebased to master and built a project locally and the versions didn't explode.

readthedocs/projects/version_handling.py Show resolved Hide resolved
@agjohnson agjohnson added the PR: work in progress Pull request is not ready for full review label Nov 13, 2018
@agjohnson agjohnson added this to the Cleanup milestone Nov 13, 2018
@humitos
Copy link
Member

humitos commented Dec 3, 2018

@agjohnson why did you mark it as "Work in progress"? It seems that it's ready to be merged.

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

This brings me great joy. Being the person who originally implemented supported versions, they never worked well for the majority of use cases, and were way too confusing for users. 💯

The only thing we need to think about here is the deployment. Because we're deleting a bunch of fields, it will break the webs during deploy, so we should make sure to deploy the webs in quick succession.

@stsewd
Copy link
Member

stsewd commented Dec 4, 2018

We need to fix the migrations

@dojutsu-user
Copy link
Member Author

I have edited and renamed the migrations file.

@ericholscher ericholscher merged commit 1b1cf36 into readthedocs:master Dec 4, 2018
@ericholscher
Copy link
Member

Thanks for this PR @dojutsu-user 💯 !

@dojutsu-user dojutsu-user deleted the remove-unused-project-model-fields branch December 4, 2018 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: work in progress Pull request is not ready for full review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants