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

Replace positional arguments with keyword arguments #1664

Merged
merged 6 commits into from
Jul 24, 2019

Conversation

vbrazo
Copy link
Member

@vbrazo vbrazo commented Jul 22, 2019

Issue

This PR is related to #1356 and #1651.

Description

Replacing positional arguments with keywords arguments.

@vbrazo vbrazo self-assigned this Jul 22, 2019
@vbrazo vbrazo changed the base branch from master to v2 July 22, 2019 19:40
@vbrazo vbrazo requested a review from stympy July 22, 2019 19:41
Copy link
Member

@bpleslie bpleslie left a comment

Choose a reason for hiding this comment

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

Looks good to me! I like this move to keyword arguments. Complex interfaces are more intuitive now.

@vbrazo vbrazo merged commit cfd730e into v2 Jul 24, 2019
@vbrazo vbrazo deleted the replace-positional-arguments-with-keyword-arguments branch July 24, 2019 02:57
@vbrazo vbrazo mentioned this pull request Jul 24, 2019
@mashedkeyboard
Copy link
Contributor

mashedkeyboard commented Aug 13, 2019

Apologies for bumping an old PR - thought it better than opening a new issue about this.

Complex interfaces are more intuitive now.

This is true in most cases - however, it doesn't make sense in all, and the "one-size-fits-all" approach that's been applied here could be improved, I think.

For instance:

# Boundary numbers are inclusive
# Keyword arguments: from, to
Faker::Number.between(from: 1, to: 10) #=> 7

# Min and Max boundaries of range are inclusive
# Keyword arguments: range
Faker::Number.within(range: 1..10) #=> 7

There's no reason for the from and two kwargs in the call to Number.between - the syntax is obvious, and it just ends up adding unnecessary extra keywords into what was already perfectly clear code. And anything that only takes one argument clearly doesn't need kwargs - that's just producing inefficiency.

The move to kwargs is a sensible one overall, but I think there might be some design decisions worth revisiting in a future update. I defer to your much greater experience building Faker than mine, though!

@byronbowerman
Copy link

anything that only takes one argument clearly doesn't need kwargs - that's just producing inefficiency.

Came here to say this as well.

@Zeragamba
Copy link
Contributor

I agree here as well. .words(number: 3) is not as nice as .words(3)

@vbrazo
Copy link
Member Author

vbrazo commented Aug 17, 2019

Yeah! We should probably centralize this discussion in an issue.

@mashedkeyboard
Copy link
Contributor

Have created #1692 to discuss this.

michebble pushed a commit to michebble/faker that referenced this pull request Feb 16, 2020
* Replace positional arguments with keyword arguments

* Update CHANGELOG entries

* Update docs
davidmorton0 pushed a commit to davidmorton0/faker that referenced this pull request Jul 12, 2021
* Replace positional arguments with keyword arguments

* Update CHANGELOG entries

* Update docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants