Skip to content

Conversation

@canonici
Copy link
Collaborator

No description provided.

@canonici canonici requested a review from ilankri August 14, 2025 13:16
@canonici canonici self-assigned this Aug 14, 2025
Copy link
Member

@ilankri ilankri left a comment

Choose a reason for hiding this comment

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

It is on the right track! The naming of the Protobuf messages and OCaml functions might need to be adjusted later.

Comment on lines +11 to +15
message Npocc {
optional string n = 1;
optional string p = 2;
optional int32 occ = 3;
}
Copy link
Member

Choose a reason for hiding this comment

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

The fields are optional to ease the switch to Proto3?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In part, but even if there is never a switch to Proto3, it can cause many issues.

https://protobuf.dev/programming-guides/proto2/#required-deprecated

Comment on lines +30 to +34
message Request {
optional int32 index = 1;
optional Npocc npocc = 2;
optional PersonRequest person_request = 3;
}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should have two distinct messages RequestByIndex and RequestByNpocc? To avoid this:

let person_select_of_piqi_request piqi_request =
let requested_index = piqi_request.Api_v2_piqi.Request.index in
let requested_npocc = piqi_request.Api_v2_piqi.Request.npocc in
match requested_index, requested_npocc with
| Some index, _ -> Some (Common.Index index)
| _, Some Api_v2_piqi.Npocc.{n = Some n; p = Some p; occ = Some occ} -> Some (Common.Npocc {n; p; occ})
| _, _ -> None

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we can, but that means two different endpoints, which is not an issue in itself. It would then be on the client to ask the right one.

src/common.ml Outdated
Comment on lines 160 to 162
if person.fields.image then
Api_util.get_portrait person.conf person.base person.person
else None
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if person.fields.image then
Api_util.get_portrait person.conf person.base person.person
else None
Ext_option.return_if person.fields.image (fun () ->
Api_util.get_portrait person.conf person.base person.person
)

src/common.ml Outdated
Comment on lines 178 to 185
if person.confidentiality.restricted then None
else
Option.bind fields (fun fields ->
Option.bind (Gwdb.get_parents person.person) (fun parents ->
let iparent = proj (Gwdb.foi person.base parents) in
let parent = Gwdb.poi person.base iparent in
Option.some (of_person person.conf person.base fields parent))
)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if person.confidentiality.restricted then None
else
Option.bind fields (fun fields ->
Option.bind (Gwdb.get_parents person.person) (fun parents ->
let iparent = proj (Gwdb.foi person.base parents) in
let parent = Gwdb.poi person.base iparent in
Option.some (of_person person.conf person.base fields parent))
)
Ext_option.return_if (not person.confidentiality.restricted (fun () ->
Option.bind fields (fun fields ->
Option.bind (Gwdb.get_parents person.person) (fun parents ->
let iparent = proj (Gwdb.foi person.base parents) in
let parent = Gwdb.poi person.base iparent in
Option.some (of_person person.conf person.base fields parent))
)
)

src/common.ml Outdated
let event_type base e = match e with
| Geneweb.Event.Pevent (Def.Epers_Name istr) -> Geneweb.Event.Pevent (Def.Epers_Name (Utf8.normalize (Gwdb.sou base istr)))
| Geneweb.Event.Fevent (Def.Efam_Name istr) -> Geneweb.Event.Fevent (Def.Efam_Name (Utf8.normalize (Gwdb.sou base istr)))
| _ as e -> (Obj.magic e)
Copy link
Member

Choose a reason for hiding this comment

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

?!

src/common.ml Outdated

type event_request = {
page_number : int;
elements_per_page : int;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
elements_per_page : int;
elements_per_page : [ `Int of int | `All ] ;

src/common.ml Outdated
Comment on lines 131 to 140
if not (has_visible_names person) then None
else
of_field person.fields.npocc (fun person' ->
let lowered_string proj person = Name.lower (as_string proj person) in
{
n = lowered_string Gwdb.get_first_name person;
p = lowered_string Gwdb.get_surname person;
occ = Int32.of_int (Gwdb.get_occ person');
}
) person
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if not (has_visible_names person) then None
else
of_field person.fields.npocc (fun person' ->
let lowered_string proj person = Name.lower (as_string proj person) in
{
n = lowered_string Gwdb.get_first_name person;
p = lowered_string Gwdb.get_surname person;
occ = Int32.of_int (Gwdb.get_occ person');
}
) person
Ext_option.return_if (has_visible_names person) (fun () ->
of_field person.fields.npocc (fun person' ->
let lowered_string proj person = Name.lower (as_string proj person) in
{
n = lowered_string Gwdb.get_first_name person;
p = lowered_string Gwdb.get_surname person;
occ = Int32.of_int (Gwdb.get_occ person');
}
) person
)

src/common.ml Outdated
Comment on lines 83 to 85
(*let as_some_string ?(none_if_empty = false) proj person =
let s = as_string proj person in
Ext_option.return_if (not none_if_empty || s <> "") (fun () -> s)*)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(*let as_some_string ?(none_if_empty = false) proj person =
let s = as_string proj person in
Ext_option.return_if (not none_if_empty || s <> "") (fun () -> s)*)

Elie Canonici Merle added 2 commits September 29, 2025 15:59
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