-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Deprecate LoremPixel #2590
Deprecate LoremPixel #2590
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @uzorjchibuzor thank you for your contribution!
When we deprecate a method, we want to maintain the behavior until the method is removed. So we don't want to comment anything, or remove any code. We just want to add a message warning users to fix the warning message, but the behavior shouldn't change until the method is removed in a future relase.
There is a gem called extend Gem::Deprecate
that you can use for this.
What it does is facilitate all of this work for us. Basically you need to extend it, and add a a "@deprecate" method above the method. I recommend checking out the gem's doc for more details.
In the LoremPixel tests, we want to test that the deprecation message is displayed when it's called.
I also realized the LoremFlick test already has a good coverage, at least at a first glance. So I will update the issue to remove this step. We definitely want to test the deprecation message warning for LoremPixel though.
Does all of this make sense?
In this case I think removeal can be done without deprecation as LoremPixel has been non-existant for quite some time now, so even if anyone was calling this generator it'd be returning non-valid urls |
It does. |
@stefannibrasil the deprecate method requires a month and year, it will not allow a string message. I am using December 2022 for now. |
I think it's safer to go through the deprecation route. Even if the generator isn't generating valid urls, it might be called somewhere and thus break things when we remove it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@uzorjchibuzor thank you for your work on this! I apologize for the late review, I was recovering from a flu 🤧
I left a suggestion. The date looks good! We can leave it as that.
@uzorjchibuzor thank you for addressing the suggestions! Once the CI is green, this is ready to merge. |
@stefannibrasil these test failures are nothing related to LoremPixel, when I checked it was coming from Sports, I will correct the rubocop offense though. |
Yeah, we can ignore the Sports one for now. There is an open issue to fix that #2608 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for your work on this!
Awesome work :) |
When Faker::LoremPixel.image is called, user is warned that
NOTE: Faker::LoremPixel.image is deprecated; use Faker::LoremFlickr.image instead. It will be removed on or after 2022-12.
@stefannibrasil
Corresponding issue #2505