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

Multiscope to main branch #118

Merged
merged 6 commits into from
Mar 21, 2024
Merged

Multiscope to main branch #118

merged 6 commits into from
Mar 21, 2024

Conversation

costero-e
Copy link
Collaborator

Hello,
as we are already implementing beacons with this accepted work in filtering terms scope, which now becomes an array of scopes, I was feeling the need to make it official, hence I am merging it to main branch. Also, I think it is good to already show it in main branch for further discussions.
Let me know what you think. Thank you.

@@ -46,6 +46,14 @@ definitions:
examples:
- 'B Lymphoblastic Leukemia/Lymphoma'
- 'Aplasia/Hypoplasia of the middle ear'
scopes:
description: >-
Entry types this filter may be applied to.
Copy link
Member

@mbaudis mbaudis Mar 20, 2024

Choose a reason for hiding this comment

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

"Entry types affected by this filter" since then we accommodate for query aggregation.

description: >-
Entry types this filter may be applied to.
examples:
- '["individual", "biosample"]'
Copy link
Member

Choose a reason for hiding this comment

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

... and making this multi-scope clear by example:

examples:
  - - individual
    - biosample
    - analysis
    - run
    - genomicVariation
  - - biosample

Copy link
Member

@mbaudis mbaudis left a comment

Choose a reason for hiding this comment

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

+1, with wish for change in wording (see inline comment).

Also, for a clean schema it would be good if the possible scopes could be derived from somewhere (i.e. defined in the model & retrieved...). Or we could make it a hard coded enum - but these are, comments, not requirements.

@redmitry
Copy link
Collaborator

it would be good if the possible scopes could be derived from somewhere

The scopes are entryTypes, So beacon provider may have its own types and associated filters...

examples:
   - - individual
     - biosample
     - analysis
     - run
     - genomicVariation
   - - biosample

I am not sure... is it valid???

I would prefer:

examples:
  - [individual, biosample, analysis, run, genomicVariation]
  - [biosample]

@mbaudis
Copy link
Member

mbaudis commented Mar 20, 2024

@redmitry

  1. Yes; therefore it would be good to have some way to retrieve the local entry types, as the maximum set of optional values. But again, this was just a thought and I don't want to expand on this ...
  2. I prefer to stay in one style (although sometimes it is easier to stay in one line if few short elements). There is even an option to have an empty indent to make the list more visible... Again, more of a comment...
examples:
   - 
     - individual
     - biosample
     - analysis
     - run
     - genomicVariation
   - 
     - biosample

(I find this version more confusing since especially w/ 2 space indentation it doesn't look that clear - but YMMV).

@costero-e
Copy link
Collaborator Author

Thank you @mbaudis and @redmitry . Please, review #119 where I have applied the changes suggested and then we will be able to merge them here, too. Thank you.

should be an array of arrays...
…msResults

Filtering terms results examples listed following tendency
@costero-e
Copy link
Collaborator Author

Changes you requested have been applied @mbaudis. If you want to review again. Thank you.

@costero-e costero-e merged commit 75850c3 into main Mar 21, 2024
1 check passed
@mbaudis
Copy link
Member

mbaudis commented Mar 21, 2024

... and it looks like that for us: http://progenetix.org/beacon/filtering_terms/?testMode=true

@costero-e costero-e added this to the 2.1 milestone Jul 16, 2024
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.

4 participants