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

Some work on jamesagnew/hapi-fhir/#1297 #1526

Merged
merged 5 commits into from Oct 22, 2019

Conversation

@jelmerterwal
Copy link
Contributor

jelmerterwal commented Oct 4, 2019

No description provided.

@jamesagnew

This comment has been minimized.

Copy link
Owner

jamesagnew commented Oct 4, 2019

Hi @jelmerterwal - Would you be able to provide some details about the issue that this PR addresses?

@jelmerterwal

This comment has been minimized.

Copy link
Contributor Author

jelmerterwal commented Oct 5, 2019

Hi @jamesagnew - This is an initial attempt to fix #1297. The example in that issue shows that it is not always possible to use equivalent Kotlin code in cases Java code would have worked.

@jamesagnew

This comment has been minimized.

Copy link
Owner

jamesagnew commented Oct 5, 2019

@jelmerterwal ah, got it.

Looks like this change is breaking CI so it probably needs some work unfortunately. Do you think it would be realistic to include a Kotlin integration test in the main HAPI build so as to provide some kind of test coverage for these changes? I'm picturing a module like hapi-fhir-server-kotlin-test that reproduces the type of setup that is in the ticket you linked.

@jelmerterwal

This comment has been minimized.

Copy link
Contributor Author

jelmerterwal commented Oct 9, 2019

@jamesagnew okay, think I see why I broke it. Hopefully this will fix the build and will add a test later today or tomorrow

@codecov

This comment has been minimized.

Copy link

codecov bot commented Oct 9, 2019

Codecov Report

Merging #1526 into master will increase coverage by 0.01%.
The diff coverage is 81.81%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1526      +/-   ##
============================================
+ Coverage     75.32%   75.34%   +0.01%     
- Complexity    12773    12779       +6     
============================================
  Files           925      925              
  Lines         52155    52146       -9     
  Branches       8727     8724       -3     
============================================
+ Hits          39288    39290       +2     
+ Misses         9399     9394       -5     
+ Partials       3468     3462       -6
Impacted Files Coverage Δ Complexity Δ
.../org/hl7/fhir/instance/model/api/IAnyResource.java 100% <ø> (ø) 1 <0> (ø) ⬇️
...src/main/java/ca/uhn/fhir/util/ReflectionUtil.java 81.92% <81.81%> (+0.67%) 22 <3> (ø) ⬇️
...jpa/validation/JpaValidationSupportChainDstu3.java 88.88% <0%> (-11.12%) 8% <0%> (-1%)
...hn/fhir/jpa/binstore/BinaryStorageInterceptor.java 95.37% <0%> (+0.17%) 33% <0%> (+2%) ⬆️
...del/entity/ResourceIndexedSearchParamQuantity.java 87.73% <0%> (+1.88%) 33% <0%> (+1%) ⬆️
.../model/entity/ResourceIndexedSearchParamToken.java 90.9% <0%> (+2.02%) 33% <0%> (+1%) ⬆️
...model/entity/ResourceIndexedSearchParamString.java 86.74% <0%> (+2.4%) 29% <0%> (+1%) ⬆️
...uhn/fhir/rest/server/method/ReadMethodBinding.java 82.27% <0%> (+2.53%) 30% <0%> (+1%) ⬆️
...ubscription/module/cache/SubscriptionRegistry.java 94.36% <0%> (+2.81%) 19% <0%> (+1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7396107...f5c1e2d. Read the comment docs.

jelmerterwal added 4 commits Oct 4, 2019
… a List in combination with Kotlin
@jelmerterwal

This comment has been minimized.

Copy link
Contributor Author

jelmerterwal commented Oct 22, 2019

@jamesagnew can you have a look?

Notes:

  • we (my colleague and I) removed some overrides since these blocked a working build, tests still work
  • we fixed a problem with CodeableConcept as well (the server would not start before the change)
@jamesagnew

This comment has been minimized.

Copy link
Owner

jamesagnew commented Oct 22, 2019

This is looking cool! Why did you scrap the hapi-fhir-server-kotlin-test though? I don't know about overloading hapi-fhir-jaxrsserver-example into being both a demo of the alternate JAX-RS server functionality and Kotlin support.. JAX-RS is not well supported as it is, and I'm worried that adding Kotlin into the mix there will further confuse people who are trying to use it as an example.

@jelmerterwal

This comment has been minimized.

Copy link
Contributor Author

jelmerterwal commented Oct 22, 2019

I see, separated the example from the Kotlin test.

@jamesagnew jamesagnew merged commit c003047 into jamesagnew:master Oct 22, 2019
5 checks passed
5 checks passed
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Java No new or fixed alerts
Details
codecov/patch 81.81% of diff hit (target 75.32%)
Details
codecov/project 75.34% (+0.01%) compared to 7396107
Details
jamesagnew.hapi-fhir Build #20191022.19 succeeded
Details
jamesagnew added a commit that referenced this pull request Oct 22, 2019
@jamesagnew

This comment has been minimized.

Copy link
Owner

jamesagnew commented Oct 22, 2019

Looks great, I've merged it. Thanks for the PR!

@jelmerterwal jelmerterwal deleted the jelmerterwal:hapi-1297 branch Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.