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

received filters in summary to object #107

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Conversation

redmitry
Copy link
Collaborator

There is an incoherence formats of filters submitted to the query and reported to be submitted:

"request summary" vs "submitted".

This pull request is to use the same filteringTerms format (object) in the beaconReceivedRequestSummary.

N.B. This would change 2.0 spec.

D.

@redmitry redmitry requested a review from mbaudis July 31, 2023 14:01
@mbaudis
Copy link
Member

mbaudis commented Jul 31, 2023

As per our discussion I'm +1 on this. However, branch management => into develop, IMO? @costero-e ? Therefore no formal approval right now.

Note to @redmitry: You also restructure the FilteringTerms document - while this shouldn't break anything (not sure about the $ref inlining; rusty on JSON Schema...) I usually would make this at least a different commit.

@mbaudis mbaudis requested a review from costero-e July 31, 2023 14:14
(postpone for another commit)
@costero-e
Copy link
Collaborator

@redmitry Why is the "id" inside brackets array? Is it because the term can apply to different scopes so you have to define it for each of the scope differently?
Answering to @mbaudis about the branch it should be merged to, if it is not a hotfix, this should be developed in a feature branch (new branch) and then make a PR to develop, indeed.

@redmitry
Copy link
Collaborator Author

@redmitry Why is the "id" inside brackets array? Is it because the term can apply to different scopes so you have to define it for each of the scope differently?

Do not get it. The only "id" inside brackets is in "required"

"required": [
                "id"
            ],

e.g.

D.

@costero-e
Copy link
Collaborator

costero-e commented Aug 1, 2023

Hey @redmitry, I meant in the examples:

"examples": [ "demographic.ethnicity:asian" ],

This would be passed as an "id" in the request but here you are specifying it has to be inside an array?

Edit: Ah, now I see is type: "string", I said nothing, sorry.

@redmitry
Copy link
Collaborator Author

redmitry commented Aug 1, 2023

Hey @redmitry, I meant in the examples:

"examples": [ "demographic.ethnicity:asian" ],

Ah! Got it.

Json Schema spec. >= draft-6
introduced standard "examples" tag to provide examples as an array of strings. We have to replace all non nomenclature "example": "" with standard "examples" : [""].

@costero-e costero-e changed the base branch from main to develop August 1, 2023 08:24
Copy link
Collaborator

@costero-e costero-e left a comment

Choose a reason for hiding this comment

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

I changed the base branch to "develop". The rest seems fine so this is approved on my side.

@jrambla
Copy link
Contributor

jrambla commented Jan 9, 2024

I understand your point, @redmitry . However the difference between request and summary is there on purpose (not a strong purpose, I must say, anyway).
The request SHOULD, of course, have a detailed description of the filter to apply, and hence the syntax must be comprehensive.
The current receivedRequestSummary, however, was not meant for machine processing, but for human inspection in case it is necessary.
This decision design is not necessarily correct and making it machine readable could be beneficial (how much? for which use case?).
However, if the PR is breaking the 2.0 specification, we SHOULD NOT added it now, but in a future release.

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.

None yet

4 participants