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.

Sign up for GitHub

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

Jump to bottom

Add Faker::Subscription #1440

Merged
merged 9 commits into from
Oct 26, 2018
Merged

Add Faker::Subscription #1440

merged 9 commits into from
Oct 26, 2018

Conversation

fabdurso
Copy link
Contributor

@fabdurso fabdurso commented Oct 24, 2018 •
edited
Loading

Changed after the comments on #1437

vbrazo
vbrazo requested changes Oct 24, 2018
View reviewed changes
Copy link
Member

@vbrazo vbrazo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Could you please add a few tests for your locales? Take a look at the comment that I left below. If you had written the locale tests, one of the tests would fail. Check the es-mx locale test file out.

lib/locales/es-MX.yml Outdated Show resolved Hide resolved
@vbrazo
Copy link
Member

vbrazo commented Oct 24, 2018

Besides es-mx, you also need to add tests for es.

@fabdurso
Copy link
Contributor Author

@vbrazo changes done, and also resolved the conflict with README. Should I change something else after 31b95c9 ?

vbrazo
vbrazo reviewed Oct 25, 2018
View reviewed changes
Copy link
Member

@vbrazo vbrazo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

We have a file in the source tree whose name is unreleased_CONTENT.md. You should include your Faker::Subscription there instead of adding it in the README.md. README.md should have only objects released in the latest version/1.9.1.

People have been confusing it for a long time, so we decided to change to see if they stop opening issues. Initially I'll have to warn contributors that we have just changed.

@fabdurso
Copy link
Contributor Author

@vbrazo should be fine now.

vbrazo
vbrazo approved these changes Oct 26, 2018
View reviewed changes
Copy link
Member

@vbrazo vbrazo left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for the hard work! 👍

@vbrazo vbrazo merged commit e244b19 into faker-ruby:master Oct 26, 2018
@fabdurso fabdurso deleted the subscription branch October 26, 2018 13:03
davidmorton0 pushed a commit to davidmorton0/faker that referenced this pull request Jul 12, 2021
@vbrazo
Add Faker::Subscription (faker-ruby#1440)
d345696 
* add Faker::Subscription

* fix test

* typo

* add tests for locales

* adding _mx to test name

* move to unreleased content

* Update CHANGELOG.md

* Update README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stympy stympy stympy approved these changes

@vbrazo vbrazo vbrazo approved these changes

Assignees

@fabdurso fabdurso

Labels
💡 Issue: Feature Request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fabdurso @vbrazo @stympy