APIv3 refactor some fields #5868
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
davidfischer
reviewed
Jul 3, 2019
readthedocs/api/v3/serializers.py
Outdated
| @@ -438,6 +450,10 @@ class Meta: | |||
| '_links', | |||
| ] | |||
|
|
|||
| def get_project_homepage(self, obj): | |||
| # Overridden only to return ``None`` when the description is ``''`` | |||
There was a problem hiding this comment.
Suggested change
| # Overridden only to return ``None`` when the description is ``''`` | |
| # Overridden only to return ``None`` when the project homepage is ``''`` |
readthedocs/api/v3/serializers.py
Outdated
| def get_project_homepage(self, obj): | ||
| # Overridden only to return ``None`` when the description is ``''`` | ||
| return obj.project_url or None | ||
|
|
||
| def get_description(self, obj): |
There was a problem hiding this comment.
This should be removed right?
readthedocs/api/v3/serializers.py
Outdated
| documentation = serializers.CharField(source='get_docs_url') | ||
| project_homepage = serializers.SerializerMethodField() | ||
| homepage = serializers.SerializerMethodField() |
There was a problem hiding this comment.
Given that we have both a "project homepage" (something set by the user) and this "homepage" (the URL on RTD) I think the latter needs a better name. I think "home" is better than "homepage" although I'm blanking on something even better than that.
There was a problem hiding this comment.
home is better than homepage, as you said. Still not perfect, though. At least, is a little more clear and consistent with the WebUI.
I will use your suggestion.
|
Updated the PR with the feedback given. It's ready to re-review and merge. |
|
Lint issues are fixed in #5857 |
It's preferable to remove it now and re-add it later once we decide if it will be completely removed from DB or not.
humitos
force-pushed
the
humitos/apiv3-fields-refactor
branch
from
099e038 to
ea3cade
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
urls.project_homepageto the root of the Project objecturlsThese changes make the object detail more clear, although I'm not sold on the name
urls--I'd like to have something more related to "Read the Docs URLs" or "URLs under Read the Docs platform". Any suggestion here?Besides the above concern, I'd like to rename
project_homepageto justhomepage(once I get a better name forurls) since it's a field of the project itself.