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.

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

Remove deprecation API for positional arguments #2573

Closed
stefannibrasil opened this issue Oct 3, 2022 · 6 comments · Fixed by #2576
Closed

Remove deprecation API for positional arguments #2573

stefannibrasil opened this issue Oct 3, 2022 · 6 comments · Fixed by #2576

Comments

@stefannibrasil
Copy link
Contributor

Version 2 of Faker replaced positional arguments with keyword arguments. To reduce users pain and make upgrades easier, deprecation warnings with instructions to upgrade were added.

And it's been a couple of years since these deprecation warnings were introduced. It's safe to remove them.

Here is what needs to be done:

@mauromorales
Copy link
Contributor

@thdaraujo could you update the tag to be hacktoberfest-accepted? Seems to be the new requirement from the event https://hacktoberfest.com/participation/#pr-mr-details

@thdaraujo
Copy link
Contributor

@thdaraujo could you update the tag to be hacktoberfest-accepted? Seems to be the new requirement from the event https://hacktoberfest.com/participation/#pr-mr-details

hey @mauromorales, thanks for submitting a PR!

Correct me if I'm wrong, but the way I understand it, the issue should have a tag hacktoberfest, and the accepted pull requests will be tagged as hacktoberfest-accepted.

Here's what they say about issues:

Apply the “hacktoberfest” label to issues you want contributors to help with in your GitHub or GitLab project.

And about PRs:

Your PR/MRs must be in a repo tagged with the “hacktoberfest” topic, or be labeled “hacktoberfest-accepted”

So when the PR is accepted, we will tag the PR as hacktoberfest-accepted.

@mauromorales
Copy link
Contributor

hey @mauromorales, thanks for submitting a PR!

My pleasure, thank you guys for making it easy to spot :)

Correct me if I'm wrong, but the way I understand it, the issue should have a tag hacktoberfest, and the accepted pull requests will be tagged as hacktoberfest-accepted.

Good question, you probably know better than me, the reason why I thought it was the other way around is because of how it looks on the hacktoberfest profile where it's labeled as not-participating but tbh is not a big deal, we can use this one as a guinea pig to find out if this is how it works or not.

Screenshot 2022-10-04 at 22 27 45

@thdaraujo
Copy link
Contributor

hey @mauromorales, thanks for submitting a PR!

My pleasure, thank you guys for making it easy to spot :)

Correct me if I'm wrong, but the way I understand it, the issue should have a tag hacktoberfest, and the accepted pull requests will be tagged as hacktoberfest-accepted.

Good question, you probably know better than me, the reason why I thought it was the other way around is because of how it looks on the hacktoberfest profile where it's labeled as not-participating but tbh is not a big deal, we can use this one as a guinea pig to find out if this is how it works or not.

Screenshot 2022-10-04 at 22 27 45

oh I see, that's probably because we did not add the topic yet to the repository, but we should have it soon! :)

@koic
Copy link
Member

koic commented Oct 7, 2022

And it's been a couple of years since these deprecation warnings were introduced. It's safe to remove them.

JFYI, Not only the migration period, so semver is also important.
https://semver.org

It should avoid making changes that could break client software on bundle update if < 3.0 specified in Gemfile or gemspec .
We saw a lot of negative voices in Faker 2.0 for this change. Let's do our best not to repeat it. So, this task is certainly easy, but it would be preferable whether the incompatible changes were done by maintainer.

That aside, it would have been a catalyst for moving forward to Faker 3.0. Thank you!

@stefannibrasil
Copy link
Contributor Author

stefannibrasil commented Oct 12, 2022

hi @koic thank you for the help! I agree it's our job as maintainers to communicate breaking changes in a way that helps users.

The reasoning behind believing that "it's been a couple of years since these deprecation warnings were introduced. It's safe to remove them." was because the keywords arguments were added before the deprecation, and a long time ago.

So reviewing the semantic version docs, I believed that since the deprecation messages were there for a long time and for a breaking change prior to it, it would be safe to remove them and fix the deprecation. I debated for a while if this was or not something major because of that. My bad, I should have raised this confusion in the issue.

To be honest, I was a bit confused by this due to the order of the deprecation. Is there something that I could have done ahead of time to make this a smooth transition, besides mentioning it as a major change in the issue? I like to learn from my mistakes, and I appreciate your support on this!

That said, I agree with making a release for Faker 3.0. @vbrazo are you available for making this release? 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment