-
Notifications
You must be signed in to change notification settings - Fork 224
feat: migrate part of existing medical provider under specific healthcare providers (#1137) #1179
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
feat: migrate part of existing medical provider under specific healthcare providers (#1137) #1179
Conversation
PR Summary
|
75c6919 to
31b8045
Compare
31b8045 to
7a9ca5e
Compare
| import net.datafaker.providers.base.AbstractProvider; | ||
|
|
||
| /** | ||
| * Generate random types of health care providers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All providers require a "since" property, so I think a since 2.3.0 would be good, even though we might release this as part of 2.2.3, not sure yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. I added it in all new classes and interface.
| } | ||
|
|
||
| /** | ||
| * @deprecated since 2.4.0. For removal in 3.0.0 version. Use {@link net.datafaker.providers.healthcare.CareProvider#medicalProfession()} instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I probably wouldn't add the "for removal in". The 3.0.0 can be released next week, or it can be done 5 years from now, that's hard to say, we usually keep these things around 1-2 releases, and then remove them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, adjusted.
src/test/java/net/datafaker/providers/healthcare/MedicalProcedureTest.java
Show resolved
Hide resolved
f56e2a0 to
991714b
Compare
|
|
||
| /** | ||
| * Generate random hospital name | ||
| * @return A hospital name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need javadoc which just repeats the actual method name here and in the rest?
IMHO: if javadoc can not bring more info than existing name then probably we don't need that javadoc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree too. However existing codebase is inconsistent, cause in some places there are such dummy javadocs, in other not. So I preferred to add it cause I didn't know which one you prefer.
So I will remove those.
| * @deprecated since 2.3.0. This faker is deprecated due to migration | ||
| * to healthcare-specific aggregated into {@link net.datafaker.providers.healthcare.HealthcareFaker} fakers with similar methods. | ||
| */ | ||
| @Deprecated(since = "2.3.0", forRemoval = true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are going to remove it
what is the replacement for Medical#diseaseName and Medical#diagnosisCode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add it in next PR, cause it touches Disease which I mentioned.
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #1179 +/- ##
============================================
- Coverage 92.35% 92.15% -0.20%
- Complexity 2821 2861 +40
============================================
Files 292 299 +7
Lines 5609 5676 +67
Branches 599 601 +2
============================================
+ Hits 5180 5231 +51
- Misses 275 292 +17
+ Partials 154 153 -1 ☔ View full report in Codecov by Sentry. |
|
I think this PR is good to go right? Any objections if I merge it? |
kingthorin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Thanks @filipowm , it's merged! |
This is initial PR to migrate existing
Medicalprovider. The next one will be to migrateBloodTypeandDiseaseproviders. Then I will move on adding new ones.I marked
Medicalprovider as deprecated since 2.4.0, yet I'm not sure whether it shouldn't be 2.3.0 or some other.