-
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
Remove support for positional arguments (deprecated on v2) #2576
Conversation
246aa2a
to
54984d1
Compare
hi @mauromorales thank you for your contribution! Could you mind pushing an empty commit to trigger the CI? For some reason, your commit didn't trigger any actions. |
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.
Looking good! I am not sure why the CI didn't run, but could you try pushing an empty commit to attempt that? Thanks!
@stefannibrasil did an empty commit, but it didn't trigger the pipeline either, maybe related to project config? Wanted to try to see if adding |
sometimes it needs someone to approve and run the pipeline. Let me get that going for ya |
awesome thanks! yup that seems to have triggered it :) |
thanks @Zeragamba I think it was because we needed to point the CI to main instead of master. It looks good now, so feel free to revert the latest two commits @mauromorales thanks! |
Co-authored-by: mauromorales <[email protected]> Co-authored-by: chimpanstache <[email protected]>
@stefannibrasil dropped the last two commits but doesn't seem to be picked 🧐 |
could you fetching your upstream branch and rebasing your branch? This commit should fix that 0581eff |
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.
LGTM, thanks for working on this! @mauromorales
Should we bump the major version as well? I'd say yes because this PR introduces breaking changes (see item 8).
We can bump the version in a different PR though.
Well, it looks like v2.0 added the breaking changes. Then v2.2.0 restored the positional arguments but added the deprecation warnings. And now we would be back to removing the functionality. 🤷♂️ So not sure if we should bump the major version or not. What are your thoughts @stefannibrasil @Zeragamba ? |
I'd call this a breaking change, and thus a v3.0 release. And since v2 was very vocal about not using positional arugments and letting people know they were deprecated, we shouldn't have to add them back in like in v2.2 |
@vbrazo we need another release 👀 |
I agree with @Zeragamba. A release containing this breaking change would mean Faker 3.0. |
Summary
This PR removes the deprecation warnings added on #1698 plus any newer ones that were added after that.
Fixes #2573