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

Feature ra plugin #392

Merged
merged 23 commits into from
Nov 30, 2021
Merged

Feature ra plugin #392

merged 23 commits into from
Nov 30, 2021

Conversation

rob-reynolds
Copy link

No description provided.

Copy link
Contributor

@JPercival JPercival left a comment

Choose a reason for hiding this comment

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

Couple of minor things I'll fix.

* @param start whether the date is the start of a period
* @return the FHIR period date as a java.util.Date type
*/
public default Date resolveRequestDate(String date, boolean start) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This code already exists in the cql-evaluator

Copy link
Author

Choose a reason for hiding this comment

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

I looked in the cql-evaluator and couldn't find it.

* @param thePort the port to use
* @return String for the client url
*/
public default String getClientUrl(String theUrlTemplate, Integer thePort) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This code isn't used anywhere?

Copy link
Author

@rob-reynolds rob-reynolds Nov 30, 2021

Choose a reason for hiding this comment

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

It's used in the ReportProviderIT. I thought it was going to be needed in non test classes too, but looks like that didn't pan out.

It could probably be moved to a test utilities.

Copy link
Author

Choose a reason for hiding this comment

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

Moved to a test-utility plugin.

dao.update(resource);
return (ResourceType) resource;
}
DaoRegistry theResourceDao, String theLocation, FhirContext theFhirContext)
Copy link
Contributor

Choose a reason for hiding this comment

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

Whoops. Meant to put this into a test utilities class.

Copy link
Author

Choose a reason for hiding this comment

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

I went back and forth whether to put this in a test utilities or in this utility. I landed on "maybe a server would want to load from a location?"

Copy link
Author

Choose a reason for hiding this comment

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

Moved to a test-utility plugin

@rob-reynolds
Copy link
Author

Couple of minor things I'll fix.

Should we fix in the PR or merge and fix later?

@rob-reynolds
Copy link
Author

Moved the test utility bits to a test-utility plugin.

It might not be perfect (like it feels like they should be excluded in a release build, etc.), but it should much closer.

@JPercival JPercival merged commit 6a0aed6 into feature-plugins Nov 30, 2021
@JPercival JPercival deleted the feature-ra-plugin branch November 30, 2021 17:38
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