Skip to content

Conversation

mambocab
Copy link
Contributor

PYTHON-893.

As mentioned on that ticket, we've decided it's better to degrade to a pseudonamedtuple (with a warning) than to simply fail on very long lists of columns.


def __getitem__(self, idx):
return self._tuple[idx]

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we just make that PseudoNamedTupleRow iterable, like a normal nametuple?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, yep, should definitely do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed -- added __iter__.

@aboudreault
Copy link
Contributor

LGTM Jim!


def pseudo_namedtuple_factory(colnames, rows):
"""
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm, wondering if we really wants to make this public ... 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the benefit to suggest users to use this instead of the dict_factory if they have the issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't given as a suggestion; it's a fallback row factory. I agree, there isn't any benefit to making this "public".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thoughts on this bit?

Copy link
Contributor Author

@mambocab mambocab left a comment

Choose a reason for hiding this comment

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

Added an __iter__ and a __repr__.

Do you think it's reasonable for this stuff to be public? I think it's ok; the documentation and naming should make it clear that you should rarely intentionally use this.


def __getitem__(self, idx):
return self._tuple[idx]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed -- added __iter__.


def pseudo_namedtuple_factory(colnames, rows):
"""
"""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thoughts on this bit?

@aboudreault
Copy link
Contributor

LGTM Jim yep!

@mambocab mambocab force-pushed the python-new-row_factory branch from b9460c9 to 0b889eb Compare June 12, 2018 14:00
@mambocab
Copy link
Contributor Author

Thanks for the review! I've added a docstring for the new factory and sqashed. Will merge after a clean test run 👍

@mambocab mambocab force-pushed the python-new-row_factory branch from 0b889eb to 16ae36b Compare June 12, 2018 20:54
@mambocab
Copy link
Contributor Author

Integration tests ran clean, travis failures will be addressed by the 3.3 removal recently merged to master. The Appveyor failures are known flakes.

@mambocab mambocab merged commit 43e85ac into master Jun 14, 2018
@mambocab mambocab deleted the python-new-row_factory branch June 14, 2018 14:39
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