Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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.

@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
# for free to join this conversation on GitHub. Already have an account? # 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