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

Feature: Return namedtuples for get_format_args #93

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

wcooley
Copy link
Contributor

@wcooley wcooley commented Sep 20, 2016

The nested tuple/list structures that are returned by get_format_args are fairly cumbersome; returning data namedtuple makes accessing the data much cleaner.

I am a bit uncertain about the field names themselves - - I chose 'positional' and 'named' because the documentation for the function describes them that way, but I chose 'key' and 'index' for the component (rather than 'position' and 'name') because those are the standard way to refer to them. It seems a bit inconsistent, however -- perhaps the FormatArgs fields should be indexed and keyed? (or "...s" instead of "...ed"?)

The nested tuple/list structures that are returned by `get_format_args`
are fairly cumbersome; returning data `namedtuple` makes accessing the
data much cleaner.
@wcooley
Copy link
Contributor Author

wcooley commented Sep 20, 2016

Also, I tested this locally with py27, py35 and pypy. (I don't have py34 and I couldn't get the py26 virtualenv to build.)

Copy link
Owner

@mahmoud mahmoud left a comment

Choose a reason for hiding this comment

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

This seems like a pretty innocuous and helpful change, thank you! I just had one change, to comply with the boltons integration architecture.

@@ -41,6 +41,7 @@
import re
from string import Formatter

from .namedutils import namedtuple
Copy link
Owner

Choose a reason for hiding this comment

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

can you change this to:

try:
    from boltons.namedutils import namedtuple
except ImportError:
    from collections import namedtuple

This will allow the module to continue to be used both inside and outside of the boltons package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. It also occurs to me that I have also neglected to update the documentation.

assert get_format_args(sample) == expected


@pytest.mark.parametrize(('sample', 'expected'), zip(_TEST_TMPLS, [
Copy link
Owner

Choose a reason for hiding this comment

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

hehe this inline parametrize is kind of long, just curious, is this common practice with pytest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume so; the pytest docs show an example of a multi-line parametrize right at the top. There are also a few instances in the pytest tests themselves (such as in test_conftest.py, test_config.py and test_mark.py).

If I have something like this that I intend to reuse exactly as-is, rather than moving the list of samples to a separate variable, I create a new mark:

pytest.mark.get_format_samples = pytest.mark.parametrize(('sample', 'expected), ...

Copy link
Owner

Choose a reason for hiding this comment

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

Interesting, 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

Successfully merging this pull request may close these issues.

2 participants