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

Fix extra argument behavior #191

Merged
merged 6 commits into from
Sep 4, 2018
Merged

Conversation

canyon289
Copy link
Member

Same question as before though! Why force people to use keyword argument?

@ColCarroll
Copy link
Member

Cool - looks like this change turned up a few legitimate bugs like the one you pointed out in #188:

************* Module arviz.tests.test_xarray_utils
arviz/tests/test_xarray_utils.py:111:15: E1121: Too many positional arguments for function call (too-many-function-args)
************* Module arviz.plots.densityplot
arviz/plots/densityplot.py:64:20: E1121: Too many positional arguments for function call (too-many-function-args)
************* Module arviz.plots.forestplot
arviz/plots/forestplot.py:161:21: E1121: Too many positional arguments for function call (too-many-function-args)

@ColCarroll
Copy link
Member

Oh, and I wanted a positional argument so the library would be more explicit and readable. It is easy to remove in the future, but hard to add back in. If it gets too annoying, we can remove!

@ahartikainen
Copy link
Contributor

It will make usage more robust. Especially important for plotting library where users have used other libs before (e.g mpl)

@ahartikainen
Copy link
Contributor

especially important if we go with multiple backends

@canyon289
Copy link
Member Author

canyon289 commented Sep 3, 2018

Oh, and I wanted a positional argument so the library would be more explicit and readable. It is easy to remove in the future, but hard to add back in. If it gets too annoying, we can remove!

You mean you wanted a keyword argument?

Makes sense and I agree, by forcing people to use keywords itll make it way easier to make changes without breaking peoples code later.

@ColCarroll
Copy link
Member

Oof, sorry, yes, keyword argument.

@canyon289 canyon289 merged commit b934e91 into arviz-devs:master Sep 4, 2018
@canyon289 canyon289 deleted the arguments branch September 6, 2018 03:32
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

Successfully merging this pull request may close these issues.

None yet

3 participants