Skip to content
This repository was archived by the owner on Mar 17, 2026. It is now read-only.

Issue/7 json complete mode#8

Merged
bogdanadrianmarc merged 22 commits into
task/infrastructure-updatefrom
issue/7-json-complete-mode
Nov 8, 2021
Merged

Issue/7 json complete mode#8
bogdanadrianmarc merged 22 commits into
task/infrastructure-updatefrom
issue/7-json-complete-mode

Conversation

@bogdanadrianmarc
Copy link
Copy Markdown
Contributor

@bogdanadrianmarc bogdanadrianmarc commented Oct 28, 2021

This PR addresses issue #7 by adding support for the complete JSON mode. The issue is that PPD uses JSON compact mode and UKHPI uses JSON complete mode, and currently the latter is not supported; this PR adds that. The repo with the new SAPINT API for LR is here: https://github.com/epimorphics/lr-data-api (I'm using the master branch). From my tests with the LR apps nothing broke by adding this and UKHPI actually works now, but please run this to double check.

@ijdickinson's suggestions from last PR did not carry over, but I will address them here in a moment

Copy link
Copy Markdown
Contributor

@ijdickinson ijdickinson left a comment

Choose a reason for hiding this comment

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

On the whole I approve of these changes, 👍

I'm still not happy about the design of json_mode_complete though. I've put in the comment some suggestions for changing the implementation of that method to something that is cleaner and more maintainable. There is another approach though, which would be to do some investigation to see how many changes would need to be made to UKHPI for it to use compact mode rather than complete mode. I have no idea off the top of my head, but it may be that the amount of work you need to do to robustly support both compact and complete modes is more than just adapting the app to use the same mode as PPD, and that would make the code more maintainable in the long run. Worth a short investigation, I would think.

Comment thread lib/data_services_api/dsapi_response_converter.rb Outdated
Comment thread lib/data_services_api/dsapi_response_converter.rb Outdated
Comment thread lib/data_services_api/dsapi_response_converter.rb
Comment thread lib/data_services_api/dsapi_response_converter.rb
Comment thread spec/data_services_api/dataset_spec.rb Outdated
Comment thread README.md
@bogdanadrianmarc bogdanadrianmarc merged commit 08025dd into task/infrastructure-update Nov 8, 2021
@jonrandahl jonrandahl deleted the issue/7-json-complete-mode branch January 13, 2023 11:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants