Migrate disease provider under healthcare providers (#1137) #1188

filipowm · 2024-05-04T06:30:39Z

Hi,

I started migrating Disease provider under healthcare, however I'm not sure if this wouldn't break backward compatibility, because:

Disease is moved from BaseProviders to HealthcareProviders
disease.yml was changed to be prepended keys with healthcare prefix.

What are your thoughts on this? Is this approach safe enough to move on?

…net#1137)

codecov-commenter · 2024-05-04T06:32:09Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.02%. Comparing base (b37c566) to head (ae00445).
Report is 141 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1188      +/-   ##
============================================
- Coverage     92.35%   92.02%   -0.33%     
- Complexity     2821     3037     +216     
============================================
  Files           292      308      +16     
  Lines          5609     5981     +372     
  Branches        599      629      +30     
============================================
+ Hits           5180     5504     +324     
- Misses          275      319      +44     
- Partials        154      158       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bodiam · 2024-05-04T14:00:50Z

To me, though I only had a quick look and didn't test it myself, this looks very good. I don't think the API is broken (maybe we should have a library API tester?), since the decease only moved, and the public API seems quite similar, so on a quick glance, it seems the API hasn't been broken.

kingthorin · 2024-05-04T14:11:41Z

Completely changing the location of methods is API breaking. Users have to re-code things to make them work again. While it is minimal effort to adapt it's still a breaking change.

snuyanzin · 2024-05-04T19:35:21Z

src/main/java/net/datafaker/providers/healthcare/Disease.java

+        List<Supplier<String>> providers = List.of(
+            this::internalDisease,
+            this::neurology,
+            this::surgery,
+            this::paediatrics,
+            this::gynecologyAndObstetrics,
+            this::ophthalmologyAndOtorhinolaryngology,
+            this::dermatology
+        );


I don't see a reason to create this list on each method call, it could be extracted in a const imho

snuyanzin · 2024-05-04T19:35:41Z

src/main/java/net/datafaker/providers/healthcare/Disease.java

+    static final String INTERNAL_DISEASE_KEY = "healthcare.disease.internal_disease";
+    static final String NEUROLOGICAL_DISEASE_KEY = "healthcare.disease.neurology";
+    static final String SURGICAL_DISEASE_KEY = "healthcare.disease.surgery";
+    static final String PAEDIATRIC_DISEASE_KEY = "healthcare.disease.paediatrics";
+    static final String GYNECOLOGY_AND_OBSTETRICS_DISEASE_KEY = "healthcare.disease.gynecology_and_obstetrics";
+    static final String OPHTHALMOLOGY_AND_OTORHINOLARYNGOLOGY_DISEASE_KEY = "healthcare.disease.ophthalmology_and_otorhinolaryngology";
+    static final String DERMATOLOGY_DISEASE_KEY = "healthcare.disease.dermatology";


Probably it's better to use enum for this

Tbh I don't see a value of enum here, as it would not add any benefit, but complicate the code.

enum DiseaseType { INTERNAL("healthcare.disease.internal_disease"); final String key; }

then

public String internalDisease() { return resolve(DiseaseType.INTERNAL.key); }

Imo it would make sense only if there is some interface which then could be passed to resolve method (resolve(KeyHolder holder)), eg.

interface KeyHolder { String key(); } enum DiseaseType implements KeyHolder { ... } public String getSomething() { return resolve(DiseaseType.ABC); }

This way it could work for other providers too easily.

I don't mind if you keep it as is, these are mostly (only?) internal things anyway, we can always refactor it later.

if you use enum, then you don't need private final List<Supplier<String>> allDiseaseProviders at all
and anyDisease could be implemented as Options.option..

And each time there will be a new disease, then with current approach you need to add both string var for this disease + maintain allDiseaseProviders. With enums you just need to update enums...
Thus finally less code

Hm, I need to play with that, cause Options.option is new to me. Thanks for that hint, sounds reasonable now.

Hi all, hope you don't mind, but I've addressed these comments in a commit on this PR, I didn't want to let these efforts go to waste. Thanks @filipowm for this improvement. @snuyanzin , can you please have another look at the PR?

src/test/java/net/datafaker/providers/healthcare/DiseaseTest.java

bodiam · 2024-05-05T09:01:16Z

Completely changing the location of methods is API breaking. Users

I might be missing something, but I don't see it? They will still do faker.disease().icd10(), why would any user care if the disease() is part of the BaseProviders or HealthcareProviders, it's still the same API call.

kingthorin · 2024-05-05T10:23:05Z

I'll have to pull the branch and play with it.

If, as you've said, the methods are still accessed the same way then I totally agree it's non-breaking.

I took bullet one from the PR description to mean that access had changed.

kingthorin · 2024-05-05T12:41:42Z

🤦‍♂️ I'm being brain dead. The unit tests would have been different if there were API changes. I know they did move but the faker usage is the same. So ignore me 😔

bodiam · 2024-05-06T11:09:52Z

The tests are failing, but not because of your changes, there still seems to be something broken in our internal works: java.lang.RuntimeException: Unable to resolve #{Internet.password '5','8','true','true'} directive for FakerContext FakerContext{, locale=SingletonLocale{locale=test}, randomService=randomService}.

kingthorin · 2024-05-06T12:07:30Z

I haven't looked but is this PR branched after that fix or does it need rebasing?

filipowm · 2024-05-06T12:54:47Z

I haven't looked but is this PR branched after that fix or does it need rebasing?

@kingthorin I branched out on saturday, so it might require rebasing. I will do that in the evening.

kingthorin · 2024-05-06T12:57:40Z

#1180 was merged "3 Days ago" (that's all the web UI shows). So hard to know.

bodiam · 2024-05-18T07:26:18Z

Hi all, what should we do with this PR?

snuyanzin · 2024-05-18T08:13:29Z

I guess addressing of this comment is still in progress #1188 (comment)
@filipowm do you need help with it?

…provide

feat: migrate disease provider under healthcare providers (datafaker-…

ed790c1

…net#1137)

snuyanzin reviewed May 4, 2024

View reviewed changes

src/test/java/net/datafaker/providers/healthcare/DiseaseTest.java Outdated Show resolved Hide resolved

chore: extract constants

75a919e

bodiam marked this pull request as draft May 25, 2024 01:37

bodiam added 2 commits June 1, 2024 12:41

Merge remote-tracking branch 'origin/main' into feat/migrate-disease-…

42e8cbc

…provide

Addressed review comments.

ae00445

bodiam marked this pull request as ready for review June 1, 2024 03:06

bodiam changed the title ~~draft: migrate disease provider under healthcare providers (#1137)~~ Migrate disease provider under healthcare providers (#1137) Jun 1, 2024

snuyanzin approved these changes Jun 1, 2024

View reviewed changes

snuyanzin merged commit 3a3e638 into datafaker-net:main Jun 1, 2024

Migrate disease provider under healthcare providers (#1137) #1188

Migrate disease provider under healthcare providers (#1137) #1188

Uh oh!

Conversation

filipowm commented May 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented May 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bodiam commented May 4, 2024

Uh oh!

kingthorin commented May 4, 2024

Uh oh!

snuyanzin May 4, 2024

Choose a reason for hiding this comment

Uh oh!

filipowm May 6, 2024

Choose a reason for hiding this comment

Uh oh!

snuyanzin May 4, 2024

Choose a reason for hiding this comment

Uh oh!

filipowm May 6, 2024

Choose a reason for hiding this comment

Uh oh!

bodiam May 6, 2024

Choose a reason for hiding this comment

Uh oh!

snuyanzin May 6, 2024 • edited by kingthorin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filipowm May 6, 2024

Choose a reason for hiding this comment

Uh oh!

bodiam Jun 1, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bodiam commented May 5, 2024

Uh oh!

kingthorin commented May 5, 2024

Uh oh!

kingthorin commented May 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bodiam commented May 6, 2024

Uh oh!

kingthorin commented May 6, 2024

Uh oh!

filipowm commented May 6, 2024

Uh oh!

kingthorin commented May 6, 2024

Uh oh!

bodiam commented May 18, 2024

Uh oh!

snuyanzin commented May 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

filipowm commented May 4, 2024 •

edited

Loading

codecov-commenter commented May 4, 2024 •

edited

Loading

snuyanzin May 6, 2024 •

edited by kingthorin

Loading

kingthorin commented May 5, 2024 •

edited

Loading

snuyanzin commented May 18, 2024 •

edited

Loading