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

Allow --patients to consume text files with IDs in it #2

Merged
merged 2 commits into from
Oct 9, 2024

Conversation

mikix
Copy link
Contributor

@mikix mikix commented Oct 9, 2024

This allows --patients to be passed either an NDJSON file (and I also had it scan for JSON Lines files whynot) or a text file that it will scan for one ID per line.

This is helpful when you have a non-Group-based cohort that you want to crawl.

I also added a commit to drop the old typescript config file, since I personally found it confusing to have both in the repo still. But if you want me to keep that file going, happy to add it back.

@vlad-ignatov
Copy link
Contributor

Code changes look great! However, I don't understand why the example config had to be removed. Its purpose is to help first-time users get started quickly by copying it and then modifying it as needed. That is also explained in the README. I think you will have to restore it, unless you have better idea of how this can be done.

@mikix
Copy link
Contributor Author

mikix commented Oct 9, 2024

However, I don't understand why the example config had to be removed.

Your confusion is a perfect example of why that file should be removed. I deleted the old (no longer referenced) v1 config (./config/example-config.ts) not the modern recommended v2 configuration file (./example-config.js). Having both is confusing, especially since we don't really support the v1 format.

@vlad-ignatov
Copy link
Contributor

Oh yes! I can't remember how I've ended up with two files 👍

Can you also remove this line and it should be ready to merge

@mikix
Copy link
Contributor Author

mikix commented Oct 9, 2024

Can you also remove this line and it should be ready to merge

Done 👍 (arguably, the config/* line could also go, since that's no longer a folder we ship/encourage anyone to use, but it's harmless)

@vlad-ignatov vlad-ignatov merged commit e7a3090 into smart-on-fhir:main Oct 9, 2024
3 checks passed
@vlad-ignatov
Copy link
Contributor

Yeah..., I am using ./config/ to store my private config files. I recommend people use a folder outside the project to avoid adding secrets to git, but I don't follow my own example because it is slightly less messy this way (easier to find things after you've forgotten what goes where...). So lets just keep that for now. Thanks!

@mikix mikix deleted the patientList branch October 9, 2024 15:17
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