Skip to content

Added pronouns provider - #1106

Merged
bodiam merged 6 commits into
datafaker-net:mainfrom
eliasnogueira:added-pronouns-provider
Mar 12, 2024
Merged

bodiam merged 6 commits into
datafaker-net:mainfrom
eliasnogueira:added-pronouns-provider

Conversation

@eliasnogueira

Copy link
Copy Markdown
Contributor

Description

This PR adds a new provider: Pronouns.

Questions

Question 1

I don't know the release process, so I added this new provider as a 2.1.0 version.
Should we change it to 2.1.1, if it gets merged, or wait until the next release?

Question 3

Should we add it as part of the Name provider?

@what-the-diff

what-the-diff Bot commented Mar 2, 2024

Copy link
Copy Markdown

PR Summary

  • Inclusion of a New Provider Class "Pronouns"
    A new provider class known as "Pronouns" has been added. This class is particularly designed with the capability to generate gender-neutral pronouns which can be used across the system.

  • Integration of "Pronouns" Class into the "BaseProviders" Class
    The newly developed "Pronouns" class has been smoothly integrated with the "BaseProviders" class. This enables the generation of pronouns as part of the data that's being created on the system.

  • Creation of a New File "pronouns.yml"
    This PR also includes the creation of a new file named "pronouns.yml". This file holds a list of gender-neutral pronouns predominantly in English.

  • Addition of a Test Class "PronounsTest"
    To verify the functionality of the newly added "Pronouns" provider, a test class labeled "PronounsTest" has been included. This will ensure the seamless operation of the new feature.

Comment thread src/main/resources/en/pronouns.yml Outdated
- "Tem"
- "Em"
possessive:
- "Her"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hers

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Comment thread src/main/resources/en/pronouns.yml Outdated
possessive:
- "Her"
- "His"
- "Their"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Theirs

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@snuyanzin

Copy link
Copy Markdown
Collaborator

it seems it should be rebased since there are conflicts

@eliasnogueira

Copy link
Copy Markdown
Contributor Author

@snuyanzin should we start a discussion about the artifact release?

From my view, this PR will get merged but not be available until you guys release a new artifact.

Should the version increase automatically and be released when the PR is merged?
Should we wait for a formal release?

I'm curious about the release process, and also happy to help.

@kingthorin

Copy link
Copy Markdown
Collaborator

On merge it'll be snapshot yes and available that way. Otherwise yes it'll have to wait for a release.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This file shouldn't be changed. 2.1.0 was already released

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seems this change was maybe done on a outdated branch?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's right. I just pushed the merge conflict fix

Comment thread docs/documentation/providers.md Outdated
| [Pokemon](https://javadoc.io/doc/net.datafaker/datafaker/latest/net/datafaker/providers/entertainment/Pokemon.html) | | Entertainment | 0.8.0 |
| [Princess Bride](https://javadoc.io/doc/net.datafaker/datafaker/latest/net/datafaker/providers/entertainment/PrincessBride.html) | | Entertainment | 0.8.0 |
| [Programming Language](https://javadoc.io/doc/net.datafaker/datafaker/latest/net/datafaker/providers/base/ProgrammingLanguage.html) | | Base | 0.8.0 |
| [Pronouns](https://javadoc.io/doc/net.datafaker/datafaker/latest/net/datafaker/providers/base/Pronouns.html) | Gender pronouns are the set of pronouns that an individual uses to reflect that person's own gender identity | Base | 2.1.0 |

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This shouldn't be modified this is part of the release tasks (IIRC)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Correct, but it doesn't hurt.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's one of the points about release. The code, doc, and version will be there without the artifact generated and publicly available.

I can follow any recommendation.


/**
* This class generates gender neutral pronouns
* @since 2.1.0

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I believe the next release is slated to be 2.2.0

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Waiting for the resolution of this comment #1106 (comment)

@kingthorin

Copy link
Copy Markdown
Collaborator

You could add it to the 'full list' in the readme.

Comment thread src/main/resources/en/pronouns.yml Outdated
subjective:
- "She"
- "He"
- "They"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm wondering if these things should be uppercased?

* This class generates gender neutral pronouns
* @since 2.1.0
*/
public class Pronouns extends AbstractProvider<BaseProviders> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any reason why a new class is used, and not add it to Gender? Gender seems like a reasonable place for this, or not?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's a good question.
I'm trying to follow the same pattern as the other " self-contained " providers. They have not implemented the fluent interface approach as I could call the class itself or another class.

Example:

faker.gender().pronouns().subjective();

The example above would be implemented in this way:

  • The Pronouns class still exists
  • The Gender class would have a pronouns() method returning its new instance
public class Gender extends AbstractProvider<BaseProviders> {

    // ignoring already existing code
    public Pronouns pronouns() {
        return new Pronouns(faker);
    }
}
  • The Pronouns class as a default toString() in the case the consumer would call only its method
faker.gender().pronouns()

I'm open to any implementation ideas :-)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@bodiam any feedback about this last comment?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

same could achieved with

    public Pronouns pronouns() {
        return faker.getPronouns();
    }

however no need to make GC happy by creation a new object each time there is a call to pronouns

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@eliasnogueira it's a bit comment, not sure what to say :-) I'm not a fan of the fluent interface approach that much, so if you want to keep the Pronouns as a separate class, that's fine.

@eliasnogueira
eliasnogueira requested a review from bodiam March 3, 2024 09:01
@codecov-commenter

codecov-commenter commented Mar 3, 2024 •

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.32%. Comparing base (b37c566) to head (abc231a).
Report is 19 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    #1106      +/-   ##
============================================
- Coverage     92.35%   92.32%   -0.03%     
- Complexity     2821     2828       +7     
============================================
  Files           292      293       +1     
  Lines          5609     5618       +9     
  Branches        599      599              
============================================
+ Hits           5180     5187       +7     
  Misses          275      275              
- Partials        154      156       +2     

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

| 2.1.0 | 4 | 237 |

Datafaker comes with a total of 233 data providers:
Datafaker comes with a total of 237 data providers:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this document is auto generated, no need to update.

@bodiam

bodiam commented Mar 12, 2024

Copy link
Copy Markdown
Contributor

Anyone objecting if I merge this?

@kingthorin

Copy link
Copy Markdown
Collaborator

Go for it

@bodiam
bodiam merged commit 28ae0a4 into datafaker-net:main Mar 12, 2024
@bodiam

bodiam commented Mar 12, 2024 •

Copy link
Copy Markdown
Contributor

Thanks @eliasnogueira et al for the work on this!

@eliasnogueira

Copy link
Copy Markdown
Contributor Author

Thank you guys!

@eliasnogueira
eliasnogueira deleted the added-pronouns-provider branch March 12, 2024 18:58
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.

5 participants