Skip to content

Conversation

@lschmierer
Copy link
Contributor

Hi @smunini, the PR contains the changes from #21 that are applicable to the main branch.
All commits should be self-contained. You can cherry-pick them individually or just merge all at once.

@lschmierer lschmierer mentioned this pull request Nov 16, 2025
@lschmierer
Copy link
Contributor Author

Sorry, the last commit does not compile. Will push a fix ASAP.

@lschmierer
Copy link
Contributor Author

Fixed. Should work now. I had to move test_serde.rs from test into src. Otherwise serde_helpers would need to be public. I think for unit tests this is fine. You might want to move simple_null_test.rs then, too.

smunini added a commit that referenced this pull request Nov 20, 2025
@smunini smunini mentioned this pull request Nov 20, 2025
@smunini
Copy link
Contributor

smunini commented Nov 20, 2025

Hi @lschmierer, thank you for this PR! Really appreciate it.

I'm going to cherry pick these changes. Here is a quick summary:

do not download r6 files if already present - I'm going to skip this one. I have evolved this area over time and there are a couple conflicting requirements that require some tradeoffs. I think the requirements surrounding this are:

  • Maintain R6 specs and generated R6 Rust code in GitHub such that the spec is downloaded each build and regenerated. Saves on load on HL7's servers, download time, and also allows a simple git clone in this repo should result in all code needed to perform a build.
  • Have a simple, documented process to regenerate R6 from build.fhir.org on-demand. That process is documented here. This is an on-demand process which I perform on occasion if there is something new/interesting in R6, or we otherwise need to regenerate all Rust code. Keeping up-to-date with R6 is often a challenge because sometimes it is a work in progress, and often test files fail.
  • Downloading a new R6 should be also tied to code generation as both are commonly done in tandem. Uncoupling these commands is described here if so desired.

I introduced the skip-r6-download feature as a means to control this download process. You introduced a check in this commit that further skips the download if files are present, which would be counter to the intended command I believe.

I did find one issue when looking at this code - I was never removing the R6 test files before extracting the newly downloaded test files - this has been fixed.

Open to other thoughts. Curious - I'm unsure what you ran that resulted in a need to add the skip download if files exist code? Was it cargo run -p helios-fhir-gen -- --all? I think this was not clear enough. I have improved the documentation - let me know if it is still unclear.

fix wrong identifiedDateTime field name - Yes! Thank you.

add AGENTS.md to .gitignore - Yes! Thank you.

make code generation deterministic - Yes! Thank you - this should be helpful going forward.

do not serialize into temporary serd_json values to detect emptiness - Yes! Thank you! I'm working on a performance test which I will post here in a separate comment when done.

Interestingly, I did find when building with the --release flag that I ran into new cycle detection optimization issues with this new code that was not there before, and needed to apply this change.

These items were cherry-picked into this branch and merged. I plan to close this PR when we are done discussing it here. Thanks again Lukas!

@smunini
Copy link
Contributor

smunini commented Nov 21, 2025

Here is the performance improvement test:

Performance Differences (Refactor vs Main):

R4: 3.00% faster (IMPROVEMENT)
R4B: No significant difference
R5: 1.00% faster (IMPROVEMENT)

R6 couldn't be compared easily because main had an old R6 test files.

@smunini smunini closed this Nov 21, 2025
@lschmierer
Copy link
Contributor Author

Open to other thoughts. Curious - I'm unsure what you ran that resulted in a need to add the skip download if files exist code? Was it cargo run -p helios-fhir-gen -- --all? I think this was not clear enough. I have improved the documentation - let me know if it is still unclear.

I honestly do not quire remember, I think it was domething like cargo run -p helios-fhir-gen --all-features.

The serde_helpers.rs file in my last commit was actually not referenced anymore, I removed it in #24

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