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

Vocabulary search common classes #56

Merged
merged 3 commits into from
Sep 11, 2023

Conversation

alex-odysseus
Copy link
Contributor

To proceed with the search in Vocabularies using Apache Solr extraction to a separate library OHDSI/WebAPI#2201

Based on the idea OHDSI/WebAPI#2196 Referencing the latest Standardized Analysis Utils and Circe 1.5.0 release preparation

…on to a separate library OHDSI/WebAPI#2201

Based on the idea OHDSI/WebAPI#2196
Referencing the latest Standardized Analysis Utils and Circe
1.5.0 release preparation
Copy link
Contributor

@chrisknoll chrisknoll left a comment

Choose a reason for hiding this comment

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

I'm glad this PR opts to introduce classes (such as SearchProviderConfig) vs. an interface that would have to be re-implemented in every downstream consumer with their own Impl class. Let's keep that going moving forward.

pom.xml Outdated
@@ -6,7 +6,7 @@

<groupId>org.ohdsi</groupId>
<artifactId>standardized-analysis-specs</artifactId>
<version>1.4.0</version>
<version>1.5.0</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be -SNAPSHOT for our dev branch. We remove the -SNAPSHOT for release.

"CONCEPT_CODE", "DOMAIN_ID", "VOCABULARY_ID", "CONCEPT_CLASS_ID",
"VALID_START_DATE", "VALID_END_DATE"
})
public class Concept extends org.ohdsi.circe.vocabulary.Concept {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is something that feels messy now: That the StandardAnalysisAPI is pulling a base class from another library (circe) but then extending it here in SAA to add ValidStartDate/ValidEndDate.

I don't think we need to take action now, but ideally what we should have in the future is that we can define standard objects in SAA (such as Concept) and haave circe-be depend back on SAA to get the 'standard' classes (along with defining it's own cohort specific classes). Eventually, it would be nice to have the circe classes put into SAA (but that is much much further down the road).

@chrisknoll chrisknoll merged commit 1da4e72 into master Sep 11, 2023
@chrisknoll chrisknoll deleted the vocabulary-search-common-classes-1.15.0 branch September 11, 2023 20:55
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.

2 participants