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

Public api #60

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Public api #60

wants to merge 9 commits into from

Conversation

EmeryYU
Copy link

@EmeryYU EmeryYU commented Apr 6, 2022

No description provided.

jacksong96 and others added 3 commits February 1, 2022 22:28
Create a new serializer: PackageInfoSerializer. A new API to return similar package info as the old kerckhoff.
Copy link
Collaborator

@changyang-liu changyang-liu left a comment

Choose a reason for hiding this comment

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

Mostly ok, the only major things are that we should add some handling for the case where a package version has not been created, and also change the JSON format a little bit.

The others are mostly minor code style changes

description = serializers.SerializerMethodField()
folder_id = serializers.SerializerMethodField()
folder_url = serializers.SerializerMethodField()
metadata = serializers.SerializerMethodField()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think just put all the obj.metadata fields into metadata instead of leaving it as an empty dictionary. And remove the ones from the top level e.g. google drive folder id and folder url

Copy link
Author

Choose a reason for hiding this comment

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

Done.


def get_data(self, obj: Package):
obj.fetch_cache()
package_items = obj.get_version(obj.latest_version.id_num).packageitem_set.all()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok when I wrote this line a few years ago I didn't realize that obj.latest_version.packageitem_set.all() is good enough.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For this one, again please add handling for the case where no versions have been created yet

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@@ -172,3 +174,82 @@ def get_version_data(self, obj: Package):
class Meta(PackageSerializer.Meta):
fields = PackageSerializer.Meta.fields + ("version_data",)
read_only_fields = PackageSerializer.Meta.read_only_fields + ("version_data",)

class PackageInfoSerializer(serializers.ModelSerializer):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd rather this be called PublicPackageSerializer

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@@ -1,3 +1,5 @@
import os
from dataclasses import field
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you meant to import this. I'm guessing it was added by the IDE

Copy link
Author

Choose a reason for hiding this comment

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

I used it for string manipulation.

from typing import List
import os
import json
from importlib_metadata import packages_distributions
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's a bunch of unused imports here. I think again it's from the IDE

Copy link
Author

Choose a reason for hiding this comment

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

Removed extra imports.


def get_description(self, obj: Package):
obj.fetch_cache()
versionSerializer = PackageVersionSerializer(obj.get_version(obj.latest_version.id_num))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add some error handling in case no versions have been created yet. Right now it will just crash the server.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually this serializer should be able to return something for packages even if they have no version, because the list packages endpoint should show every package even if no version has been created

Copy link
Author

Choose a reason for hiding this comment

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

Done.

file_ext = os.path.splitext(file.file_name)[-1]
if(file.file_name == "article.aml"):
aml_data = file.data["content_rich"]["data"]
if(file_ext in supported_image_types):
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't seem to have included the images into the JSON data. I think you should either get rid of this code, or just put them into the JSON as well.

Copy link
Author

Choose a reason for hiding this comment

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

I kept these code because I guessed we would include images somewhere later, but I just deleted these for now as you asked.

EmeryYU added 6 commits May 10, 2022 14:29
Allows for retrieving data and images form a specified version.
This reverts commit 5f1e299.
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.

3 participants