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

Using kwargs for methods that accept only one argument is inefficient #1692

Closed
mashedkeyboard opened this issue Aug 17, 2019 · 4 comments
Closed

Comments

@mashedkeyboard
Copy link
Contributor

Following on from a fair bit of noise in #1664.

Unfortunately, I think a lot of the people impacted by this aren't really involved in the development of faker, and so only came to the GitHub when this came up (after when the change was merged!)

That said, as mentioned in the PR discussion, things like .words(3) being turned into .words(number: 3) is only adding more syntax where less would do just fine. The meaning of .words(3) is totally clear - keyword arguments work great for more complex Faker methods like .password which accepts 4 arguments that aren't immediately obvious from context, but uniformly applying them to all methods is counterproductive somewhat.

I propose that all single argument and some two-argument methods have their kwargs removed and replaced by normal positional arguments in a future release.

@stympy
Copy link
Contributor

stympy commented Aug 20, 2019

I agree with the proposal :)

@vbrazo
Copy link
Member

vbrazo commented Sep 22, 2019

@mashedkeyboard it makes sense to me. Feel free to work on that.

@koic
Copy link
Member

koic commented Sep 24, 2019

I almost agree with this proposal. On the other hand, I have heard an opinion that agree with the introduction of keyword arguments even for a single argument (Faker 2 kwargs style) , and may have some design preference.

So, these interfaces may need to be carefully discussed.

And, I think this will be a major published API change following Faker 2.
It is preferable to output a warning in advance to reduce user impact if published API changes.
For example, the following milestones can be considered.

  • Faker 2.x ... Display deprecation warning when executing API
  • Faker 3.0 ... Remove deprecation API

@thdaraujo
Copy link
Contributor

Hey, folks. In an effort to lighten our load as maintainers and be able to serve you better in the future, the faker-ruby team is working on cleaning out the cobwebs in this repo by pruning the backlog. As there are few of us, there are a lot of items that will simply never earn our attention in a reasonable time frame, and rather than giving you an empty promise, we think it makes more sense to focus on more recent issues. That means, unfortunately, that we must close this issue.

Don't take this the wrong way: our aim is not to diminish the effort people have made or dismiss problems that have been raised. If you feel that we should reopen this issue, then please let us know so that we can reprioritize it. Thanks!

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

No branches or pull requests

5 participants