Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 61 additions & 0 deletions ebl/signs/infrastructure/mongo_sign_repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,12 @@ def filter_none(self, data, **kwargs):
return {key: value for key, value in data.items() if value is not None}


class OrderedSignSchema(Schema):
name = fields.String(required=True)
unicode = fields.List(fields.Int(), required=True)
sort_key = fields.Int(required=True)


class LogogramSchema(Schema):
logogram = fields.String(required=True)
atf = fields.String(required=True)
Expand Down Expand Up @@ -101,6 +107,10 @@ def make_sign(self, data, **kwargs) -> Sign:
return Sign(**data)


class OrderedSignsSchema(Schema):
ordered_signs = fields.Nested(OrderedSignSchema, many=True, data_key="signs")


class SignDtoSchema(SignSchema):
@post_dump
def make_sign_dto(self, data, **kwargs) -> Dict:
Expand All @@ -123,6 +133,57 @@ def find(self, name: SignName) -> Sign:
data = self._collection.find_one_by_id(name)
return cast(Sign, SignSchema(unknown=EXCLUDE).load(data))

def find_signs_by_order(self, name: SignName, order: str, sort_era: str) -> Sign:

Choose a reason for hiding this comment

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

issue (code_clarification): Return type of 'find_signs_by_order' should be List[Sign] instead of Sign.

The method appears to return a list of signs, not a single sign, as indicated by the aggregation and grouping in the MongoDB query.

Choose a reason for hiding this comment

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

Is this comment correct?

Choose a reason for hiding this comment

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

Is this comment helpful?

Choose a reason for hiding this comment

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

Is the comment type correct?

Choose a reason for hiding this comment

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

Is the comment area correct?

Choose a reason for hiding this comment

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

Should this comment become an LLM test?

key = self._collection.find_one_by_id(name)["sortKeys"][sort_era][0]

Choose a reason for hiding this comment

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

issue (edge_case_not_handled): Potential risk of KeyError if 'sortKeys' or 'sort_era' does not exist.

Consider adding error handling or checks to ensure the keys exist before accessing them to avoid runtime errors.

Choose a reason for hiding this comment

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

Is this comment correct?

Choose a reason for hiding this comment

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

Is this comment helpful?

Choose a reason for hiding this comment

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

Is the comment type correct?

Choose a reason for hiding this comment

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

Is the comment area correct?

Choose a reason for hiding this comment

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

Should this comment become an LLM test?

range_start = "$lt"
range_end = "$gte"
end_key = key - 5
if order == "following":
range_start = "$gt"
range_end = "$lte"
end_key = key + 5
cursor = self._collection.aggregate(
[
{
"$match": {
f"sortKeys.{sort_era}": {
"$elemMatch": {
f"{range_start}": key,
f"{range_end}": end_key,
}
}
}
},
{"$unwind": f"$sortKeys.{sort_era}"},
{
"$match": {
f"sortKeys.{sort_era}": {
f"{range_start}": key,
f"{range_end}": end_key,
}
}
},
{
"$project": {
"sign": 1,
"unicode": 1,
"name": "$_id",
"sort_key": f"$sortKeys.{sort_era}",
}
},
{"$sort": {"sort_key": 1}},
{"$group": {"_id": "1", "signs": {"$push": "$$ROOT"}}},
{
"$project": {
"signs.name": 1,
"signs.unicode": 1,
"signs.sort_key": 1,
}
},
]
)
return OrderedSignsSchema().load(cursor, unknown=EXCLUDE, many=True)

def search(self, reading: str, sub_index: Optional[int] = None) -> Optional[Sign]:
sub_index_query = {"$exists": False} if sub_index is None else sub_index
try:
Expand Down
4 changes: 3 additions & 1 deletion ebl/signs/web/bootstrap.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,14 @@
CroppedAnnotationService,
)
from ebl.signs.web.sign_search import SignsSearch
from ebl.signs.web.signs import SignsResource, SignsListResource
from ebl.signs.web.signs import SignsResource, SignsListResource, SignsOrderResource
from ebl.signs.web.cropped_annotations import CroppedAnnotationsResource


def create_signs_routes(api: falcon.App, context: Context):
signs_search = SignsSearch(context.sign_repository)
signs = SignsResource(context.sign_repository)
ordered_signs = SignsOrderResource(context.sign_repository)
signs_images = CroppedAnnotationsResource(
CroppedAnnotationService(
context.annotations_repository,
Expand All @@ -24,3 +25,4 @@ def create_signs_routes(api: falcon.App, context: Context):
api.add_route("/signs/{sign_name}", signs)
api.add_route("/signs/{sign_name}/images", signs_images)
api.add_route("/signs/all", signs_all)
api.add_route("/signs/{sign_name}/{order}/{sort_era}", ordered_signs)
10 changes: 10 additions & 0 deletions ebl/signs/web/signs.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,3 +35,13 @@ def __init__(self, signs: SignRepository):

def on_get(self, req, resp):
resp.media = self.sign_repository.list_all_signs()


class SignsOrderResource:
def __init__(self, signs: SignRepository):
self.sign_repository = signs

def on_get(self, req, resp, sign_name, order, sort_era):
resp.media = self.sign_repository.find_signs_by_order(
sign_name, order, sort_era
Comment on lines +44 to +46

Choose a reason for hiding this comment

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

issue (testing): Missing test cases for the new endpoint.

The PR introduces a new endpoint but does not include any tests for it. It's crucial to add unit and integration tests to verify that the endpoint behaves as expected under various scenarios, including edge cases and error handling.

Choose a reason for hiding this comment

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

Is this comment correct?

Choose a reason for hiding this comment

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

Is this comment helpful?

Choose a reason for hiding this comment

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

Is the comment type correct?

Choose a reason for hiding this comment

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

Is the comment area correct?

Choose a reason for hiding this comment

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

Should this comment become an LLM test?

)