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

APIv3 refactor some fields #5868

Merged
merged 7 commits into from Jul 15, 2019
Merged

APIv3 refactor some fields #5868

merged 7 commits into from Jul 15, 2019

Conversation

@humitos
Copy link
Member

@humitos humitos commented Jul 1, 2019

  • Move urls.project_homepage to the root of the Project object
  • Add Project, Builds, Versions and Documentation (all URLs under Read the Docs) in urls

These 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_homepage to just homepage (once I get a better name for urls) since it's a field of the project itself.

@humitos humitos requested a review from Jul 1, 2019
@@ -438,6 +450,10 @@ class Meta:
'_links',
]

def get_project_homepage(self, obj):
# Overridden only to return ``None`` when the description is ``''``
Copy link
Contributor

@davidfischer davidfischer Jul 3, 2019

Suggested change
# Overridden only to return ``None`` when the description is ``''``
# Overridden only to return ``None`` when the project homepage is ``''``

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):
Copy link
Contributor

@davidfischer davidfischer Jul 3, 2019

This should be removed right?

documentation = serializers.CharField(source='get_docs_url')
project_homepage = serializers.SerializerMethodField()
homepage = serializers.SerializerMethodField()
Copy link
Contributor

@davidfischer davidfischer Jul 3, 2019

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.

Screen Shot 2019-07-03 at 9 28 23 AM

Copy link
Member Author

@humitos humitos Jul 8, 2019

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.

@humitos
Copy link
Member Author

@humitos humitos commented Jul 8, 2019

Updated the PR with the feedback given. It's ready to re-review and merge.

@humitos
Copy link
Member Author

@humitos humitos commented Jul 8, 2019

Lint issues are fixed in #5857

@humitos humitos force-pushed the humitos/apiv3-fields-refactor branch from 099e038 to ea3cade Jul 9, 2019
@humitos humitos requested a review from davidfischer Jul 9, 2019
Copy link
Member

@ericholscher ericholscher left a comment

Makes sense 👍

@ericholscher ericholscher merged commit 29a956d into master Jul 15, 2019
1 check passed
@delete-merged-branch delete-merged-branch bot deleted the humitos/apiv3-fields-refactor branch Jul 15, 2019
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