Skip to content

Fix debugging output for nameless py::arg_v annotations #648

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

Merged
merged 3 commits into from
Feb 8, 2017

Conversation

jagerman
Copy link
Member

@jagerman jagerman commented Feb 6, 2017

This fixes a couple bugs with nameless py::arg_v (introduced in #634) annotations:

  • the argument name was being used in debug mode without checking that it exists (which would result in the std::string construction throwing an exception for being invoked with a nullptr)
  • the error output says "keyword arguments", but py::arg_v() can now also be used for positional argument defaults.
  • the debugging output "in function named 'blah'" was overly verbose: changed it to just "in function 'blah'".

This fixes a couple bugs with nameless py::arg() (introduced in pybind#634)
annotations:

- the argument name was being used in debug mode without checking that
  it exists (which would result in the std::string construction throwing
  an exception for being invoked with a nullptr)
- the error output says "keyword arguments", but py::arg_v() can now
  also be used for positional argument defaults.
- the debugging output "in function named 'blah'" was overly verbose:
  changed it to just "in function 'blah'".
@dean0x7d
Copy link
Member

dean0x7d commented Feb 7, 2017

What's the rationale for adding new tests to test_issues instead of to the relevant test_* files? It seems like the tests/issues split makes it harder to located all related tests.

@wjakob
Copy link
Member

wjakob commented Feb 7, 2017

@dean0x7d: it's historical: the examples used to be actual examples, and it felt weird to move issue code into those files which were meant as additional demonstrations of certain binding features.

@wjakob
Copy link
Member

wjakob commented Feb 7, 2017

(this was from a time when this project really wasn't all that well-documented)

@wjakob
Copy link
Member

wjakob commented Feb 7, 2017

@dean0x7d: Ah, sorry -- I misunderstood (you were referring to @jagerman's commit). I thought that you were generally questioning the idea of putting tests into test_issues.cpp

@dean0x7d
Copy link
Member

dean0x7d commented Feb 7, 2017

Ah, sorry -- I misunderstood (you were referring to @jagerman's commit). I thought that you were generally questioning the idea of putting tests into test_issues.cpp

A little bit of both I guess. I understand the historical reason for the split, but I'm just suggesting that it would be nicer to group by functionality going forward (for ease of maintenance) since these are definitely not examples anymore.

@jagerman
Copy link
Member Author

jagerman commented Feb 8, 2017

No idea what happened to the last test--looks like a appveyor problem (the first three passed).

@wjakob
Copy link
Member

wjakob commented Feb 8, 2017

I'll merge it -- let's see what happens with AppVeyor :)

@wjakob wjakob merged commit 1eaacd1 into pybind:master Feb 8, 2017
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.

3 participants