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

Filtering terms in model fix #161

Closed
wants to merge 2 commits into from

Conversation

mbaudis
Copy link
Member

@mbaudis mbaudis commented Aug 21, 2024

The previous filteringTerms.yaml/json endpoint definitions were odd:

  • either defined as empty lists (well, okish)
  • or having some random values

This PR redefines filteringTerms endpoints as containing a list of

https://raw.githubusercontent.com/ga4gh-beacon/beacon-v2/main/framework/json/responses/sections/beaconFilteringTermsResults.json#/definitions/FilteringTerm

items. Should be non-breaking.

Thanks to @gsfk for repeatedly voicing concerns about filteringTerms in general ...

NOTE: Some of the changes in the diffs seem to come from previous direct edits on json files instead of proper edit in yaml -> run yamlerrunner.sh -> ordered JSON ...

The previous filteringTerms.yaml/json endpoint definitions were odd:
 - either defined as empty lists (well, okish)
- or having some random values

This PR redefines filteringTerms endpoints as containing a list of

https://raw.githubusercontent.com/ga4gh-beacon/beacon-v2/main/framework/json/responses/sections/beaconFilteringTermsResults.json#/definitions/FilteringTerm

items. Should be non-breaking.
@mbaudis mbaudis requested review from costero-e and jrambla August 21, 2024 19:29
@gsfk
Copy link
Collaborator

gsfk commented Aug 21, 2024

Are these files used anywhere? Is there a compelling reason to keep them?

@mbaudis
Copy link
Member Author

mbaudis commented Aug 22, 2024

Are these files used anywhere? Is there a compelling reason to keep them?

@gsfk They represent the endpoints for the entity specific filteringTerms results but are basically stand-ins for resource specific solutions. One could argue that they are defined in openAPI endpoints - but we actually want to transition away from those & need some place to indicate that something like these exists.

That being said, I do not think that the files/definitions are used directly (one of the reasons nobody has cared about their content...).

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.

First of all, I defined a new setting for schema-urgent-fixes branch which is to require that all branches must be updated with last commits to schema-urgent-fixes, because I was seeing a lot of changes related to undo commits that were already merged to this branch that address and fix things, so this was dangerous as schema-urgent-fixes is being used a lot now and will serve later to have a lot of things addressed in a PR to main/develop.
On the other hand, now that I clearly see what was this PR about, I see that the $ref in items for the src files point to the beaconFilteringTermsResults.json file and should be addressed by pointing to the .yaml file, which is https://raw.githubusercontent.com/ga4gh-beacon/beacon-v2/main/framework/src/responses/sections/beaconFilteringTermsResults.yaml.

@costero-e
Copy link
Collaborator

With merge of #165 this is no longer mergeable. Shall I close it, @mbaudis ?

@costero-e
Copy link
Collaborator

This has been merged in #161

@costero-e costero-e closed this Dec 12, 2024
@costero-e costero-e deleted the filtering-terms-in-model-fix branch December 13, 2024 08:47
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