Added medical extension to core #232

pimjansen · 2020-12-22T14:39:19Z

What is the reason for this PR?

Medical added as Core Extension

A new feature
Fixed an issue (resolve #ID)

Author's checklist

Follow the Contribution Guide
New features and changes are documented

Summary of changes

Medical added as Core Extension

Review checklist

All checks have passed
Changes are approved by maintainer

src/Faker/Generator.php

Nyholm · 2020-12-22T14:44:32Z

This would be a good example of a super standard extension.

pimjansen · 2020-12-22T14:46:05Z

This would be a good example of a super standard extension.

Tbh i don't see added value in adding extensions for everything. Since the fact that this is not language specific and generic we could just leave it in Core?

Size wise it is not an issue where the only real problems there are with the actual realText methods

Nyholm · 2020-12-22T14:49:15Z

Tbh i don't see added value in adding extensions for everything. Since the fact that this is not language specific and generic we could just leave it in Core?

Just a terminology check. The Faker\Core\Medical is an "extension". Is it also a "Core extension". I agree with you that the German package will not have a class that implements Faker\Extension\MedicalExtension. But that is fine.

We need to add an extension interface to everything that has a "helper method" in Generator.

localheinz · 2020-12-22T14:48:44Z

src/Faker/Core/Medical.php

+    /**
+     * @inheritDoc
+     */


Suggested change

/**

* @inheritDoc

*/

Can we remove this?

This information is automatically inherited, and these DocBlocks are generally useless.

localheinz · 2020-12-22T14:53:49Z

test/Faker/Core/MedicalTest.php

+{
+    public function testBloodType(): void
+    {
+        self::assertContains($this->faker->bloodType(), ['A', 'AB', 'B', 'O']);


Can we use a more appropriate assertion?

As far as I am concerned, when using assertContains(), then $needle would be the expected value here, whereas $haystack is the actual value.

Right now it is the other way around.

The needle is always the value and the haystack the expected? In this case needle also needs to be a string.

This is a 1:1 copy from the existing tests so i wonder where you want to?

localheinz · 2020-12-22T14:54:47Z

src/Faker/Core/Medical.php

+/**
+ * @experimental This class is experimental and does not fall under our BC promise
+ */
+final class Medical implements Extension\MedicalExtension


Perhaps a Blood extension would make more sense?

All of the methods in this class share the blood prefix.

Yeah, that would make more sense.

Renamed and reword to Blood instead

pimjansen · 2020-12-22T15:43:45Z

Understand what you mean @Nyholm. What approach will we take here since i do not really see value in splitting the not language specific things as well. Or will we put the code in Faker\Extension\Medical itself

krsriq · 2020-12-22T16:00:35Z

@pimjansen - I think it's fine to use a Faker\Extension interface for every extension.
Who knows, it might be that blood types in fact need to be localized in some circumstances - for example some countries seem to use 0 instead of O, as was mentioned in #61.

Nyholm · 2020-12-22T16:05:28Z

What approach will we take here since i do not really see value in splitting the not language specific things as well.

I argue that we dont need to add this extension to a language specific package.

Or will we put the code in Faker\Extension\Medical itself

No, never, we must use an interface because we define methods in Generator

Nyholm · 2020-12-22T16:22:45Z

src/Faker/Extension/ContainerBuilder.php

@@ -62,6 +62,7 @@ public static function defaultExtensions(): array
    {
        return [
            FileExtension::class => Core\File::class,
+            MedicalExtension::class => Core\Medical::class,


When you rename this to BloodExtension, then make sure this is in alphabetical order. It would please my OCD =)

How about Medical\BloodExtension?

Looking at the list of the current features we have, there is nothing else that would fit in Medical namespace.

We could add a list of deceases, but that would be localized.

I would rather keep the list of features as small as possible.

I am more than happy to add all kinds of medical related stuff in a separate package. Ie fakerphp/medical.

Lets focus on the current features and this PR at the moment and later we can discuss if this belongs to core or not

I would like to keep this in a fakerphp/medical, this doesn’t feel like something that every application would need. Which is kind of the point of our “core”, to keep general and commonly needed extensions.

I doubt that last one a bit since it brings the ease of use. Nobody wants to have 6 extra dependencies for a package like this.

I ofc agree to split of all the language specifics where it is impossible to maintain centralised and also all the language packs holds a lot of storage but for all other generic stuff?

Since for example when i need to create a dummy person with all its info i need a for example Swedish pack, Faker core and a medical extension to add its blood type

Nyholm · 2020-12-26T11:53:22Z

Pim, could you please rename this extension to Blood?

bram-pkg · 2020-12-26T12:07:48Z

src/Faker/Extension/ContainerBuilder.php

@@ -62,6 +62,7 @@ public static function defaultExtensions(): array
    {
        return [
            FileExtension::class => Core\File::class,
+            MedicalExtension::class => Core\Medical::class,


I would like to keep this in a fakerphp/medical, this doesn’t feel like something that every application would need. Which is kind of the point of our “core”, to keep general and commonly needed extensions.

pimjansen · 2020-12-26T12:30:52Z

Pim, could you please rename this extension to Blood?

Yeah will update and include all the comments somewhere this week

Nyholm

Thank you. I am happy with this PR.

pimjansen commented Dec 22, 2020

View reviewed changes

src/Faker/Generator.php Show resolved Hide resolved

pimjansen added the enhancement New feature or request label Dec 22, 2020

Nyholm requested review from bram-pkg, GrahamCampbell and localheinz December 22, 2020 14:44

localheinz reviewed Dec 22, 2020

View reviewed changes

Nyholm reviewed Dec 22, 2020

View reviewed changes

bram-pkg requested changes Dec 26, 2020

View reviewed changes

Added blood extension to core

8f1badd

pimjansen force-pushed the feature/medical-core-extension branch from 862cbed to 8f1badd Compare December 27, 2020 13:55

Nyholm approved these changes Dec 30, 2020

View reviewed changes

pimjansen merged commit 8645d0c into FakerPHP:main Dec 30, 2020

pimjansen deleted the feature/medical-core-extension branch December 30, 2020 16:50

Nyholm mentioned this pull request Jan 7, 2021

Feature: Add Language name #132

Closed

krsriq mentioned this pull request Jan 7, 2021

Added barcode extension to core #252

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added medical extension to core #232

Added medical extension to core #232

pimjansen commented Dec 22, 2020 •

edited

Loading

Nyholm commented Dec 22, 2020

pimjansen commented Dec 22, 2020

Nyholm commented Dec 22, 2020

localheinz Dec 22, 2020

localheinz Dec 22, 2020

pimjansen Dec 28, 2020

localheinz Dec 22, 2020

Nyholm Dec 22, 2020

pimjansen Dec 28, 2020

pimjansen commented Dec 22, 2020

krsriq commented Dec 22, 2020

Nyholm commented Dec 22, 2020

Nyholm Dec 22, 2020

localheinz Dec 22, 2020

Nyholm Dec 22, 2020

bram-pkg Dec 22, 2020

Nyholm Dec 22, 2020

bram-pkg Dec 26, 2020

pimjansen Dec 28, 2020

Nyholm commented Dec 26, 2020

bram-pkg Dec 26, 2020

pimjansen commented Dec 26, 2020

Nyholm left a comment

Added medical extension to core #232

Added medical extension to core #232

Conversation

pimjansen commented Dec 22, 2020 • edited Loading

What is the reason for this PR?

Author's checklist

Summary of changes

Review checklist

Nyholm commented Dec 22, 2020

pimjansen commented Dec 22, 2020

Nyholm commented Dec 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pimjansen commented Dec 22, 2020

krsriq commented Dec 22, 2020

Nyholm commented Dec 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm commented Dec 26, 2020

Choose a reason for hiding this comment

pimjansen commented Dec 26, 2020

Nyholm left a comment

Choose a reason for hiding this comment

pimjansen commented Dec 22, 2020 •

edited

Loading