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

Remove unneeded require in lib/faker.rb #2558

Merged
merged 1 commit into from
Sep 23, 2022
Merged

Remove unneeded require in lib/faker.rb #2558

merged 1 commit into from
Sep 23, 2022

Conversation

dmarcoux
Copy link
Contributor

@dmarcoux dmarcoux commented Sep 3, 2022 •
edited
Loading

Summary

The version of the dependency i18n has to be at least 1.8.11, so this require can be safely removed.

Fixes Issue #2524

Remove unneeded require in lib/faker.rb
aed3d37 
The version of the dependency i18n has to be at least 1.8.11, so this
`require` can be safely removed.
@dmarcoux
Copy link
Contributor Author

dmarcoux commented Sep 3, 2022

Does something have to be done regarding this? I'm asking since it was mentioned in #2524, but I'm missing context to fully understand what should be done here.

@thdaraujo
Copy link
Contributor

thdaraujo commented Sep 12, 2022 •
edited
Loading

Thanks for working on this @dmarcoux !

To answer your question: we would like to get rid of this comment too as it doesn't seem to be relevant anymore:

faker/faker.gemspec

Line 30 in 65e5bb9

# Requires Ruby I18n 1.8.11 or higher to resolve https://github.com/faker-ruby/faker/issues/2330.

@thdaraujo thdaraujo self-requested a review September 12, 2022 19:03
thdaraujo
thdaraujo requested changes Sep 12, 2022
View reviewed changes
Copy link
Contributor

@thdaraujo thdaraujo left a comment

Choose a reason for hiding this comment

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

LGTM. Let's also remove this comment:

faker/faker.gemspec

Line 30 in 65e5bb9

# Requires Ruby I18n 1.8.11 or higher to resolve https://github.com/faker-ruby/faker/issues/2330.

@stefannibrasil stefannibrasil linked an issue Sep 15, 2022 that may be closed by this pull request
Resolve issue with i18n version #2524
Closed
@stefannibrasil
Copy link
Contributor

thank you @dmarcoux it looks like I18n is updated but that comment wasn't removed. Please go ahead and remove that comment, then this one is good to go 🚀

stefannibrasil
stefannibrasil reviewed Sep 23, 2022
View reviewed changes
Copy link
Contributor

@stefannibrasil stefannibrasil 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 for now. Thanks @dmarcoux 💯

@stefannibrasil stefannibrasil merged commit b77a5c6 into faker-ruby:master Sep 23, 2022
@stefannibrasil stefannibrasil linked an issue Oct 5, 2022 that may be closed by this pull request
[Backwards Breaking change 2.22] Faker is reliably generating duplicate values #2534
Closed
@stefannibrasil stefannibrasil removed a link to an issue Oct 5, 2022
[Backwards Breaking change 2.22] Faker is reliably generating duplicate values #2534
Closed
sudeeptarlekar pushed a commit to sudeeptarlekar/faker that referenced this pull request Oct 7, 2022
@sudeeptarlekar
Remove unneeded require in lib/faker.rb (faker-ruby#2558)
239b1c3 
The version of the dependency i18n has to be at least 1.8.11, so this
`require` can be safely removed.
@dmarcoux dmarcoux deleted the i18n branch October 12, 2022 18:45
@dmarcoux
Copy link
Contributor Author

Thank you for handling this, I was away for some time, so I couldn't act on your comments.

@dmarcoux dmarcoux removed their assignment Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stefannibrasil stefannibrasil stefannibrasil left review comments

@thdaraujo thdaraujo thdaraujo requested changes

Assignees
No one assigned
Labels
Chores ⚒️ Improvement 🏃🏽 PR: In Progress 🙏🏽 PR: Needs Further Updates
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Resolve issue with i18n version
3 participants
@dmarcoux @thdaraujo @stefannibrasil