8000 Fix crash in FFaker::Time#datetime when defining FFaker::Date module by brupla6126 · Pull Request #521 · ffaker/ffaker · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix crash in FFaker::Time#datetime when defining FFaker::Date module #521

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.

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

Merged

Conversation

brupla6126
Copy link
Contributor
@brupla6126 brupla6126 commented May 27, 2022

Defining the FFaker::Date module makes FFaker::Time#datetime crash.

When migrating from Faker, we needed to add the module FFaker::Date to handle #backward and #forward differently than FFaker::Time.

FFaker::Time#datetime uses Date.new instead of ::Date.new so it crashes on trying to instantiate FFaker::Date which is a module.

Comment on lines 8 to 9
extend FFaker::ModuleUtils
extend self
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it live somewhere else? I mean, not in test folder

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe something like ::Date is meaning here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, I don't get the idea putting this module to test file. Overall I understand the issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't it live somewhere else? I mean, not in test folder

I put the module there just to demonstrate the bug. I'll implement the FFaker::Date module with the methods we needed from our migration from Faker to FFaker.

Fix bugs with ::Date module clashing with FFaker::Date.
@brupla6126 brupla6126 requested review from AlexWayfer and Volosh1n May 31, 2022 13:48
Copy link
Contributor
@Volosh1n Volosh1n left a comment

Choose a reason for hiding this comment

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

👍

@Volosh1n Volosh1n requested a review from marocchino May 31, 2022 14:10
Copy link
Member
@marocchino marocchino left a comment

Choose a reason for hiding this comment

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

👍🏽

@marocchino marocchino merged commit 990466b into ffaker:main May 31, 2022
@brupla6126 brupla6126 deleted the Fix_Time_datetime_when_defining_module_date branch May 31, 2022 14:30
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.

4 participants
0