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

Pass reader error messages along #28

Merged
merged 2 commits into from
May 22, 2018

Conversation

mpeteuil
Copy link
Contributor

This fixes problems like the one mentioned in issue #27 where using the reader in Python 2.x with it's default str type would throw an error, but the error message was overridden. There was a similar change made for issue #5.

I don't believe a new test is required for this since the TestDialectValidity.test_delimiter test covers the source exception and this is more about the messaging bubbling up. It would be possible if needed to check the message higher up the chain though.

This fixes problems like the one mentioned in issue ryanhiebert#27 where using the
reader in Python 2.x with it's default `str` type would throw an error,
but the error message was overridden. There was a similar change made
for issue ryanhiebert#5.

I don't believe a new test is required for this since
https://github.com/ryanhiebert/backports.csv/blob/1.0.5/tests.py#L853-L856
covers the source exception and this is more about the messaging
bubbling up. It would be possible if needed to check the message higher
up the chain though.
@mpeteuil mpeteuil force-pushed the reader-error-messages branch from 3e04f20 to c17be7d Compare May 19, 2018 13:08
@ryanhiebert
Copy link
Owner

Since the tests are still passing and no new behavior or branching is being added, I agree that we shouldn't need to add additional tests. It looks like virtualenv dropped support for Python 3.3 (via assuming wheel support for all Python 3), so in order to get this build passing I'll need to drop support for Python 3.3. That's not related to this PR, just exposed by this PR.

I imagine that it might also be possible to retain Python 3.3 compatibility by pinning to an older version of wheel as a test dependency. I might try that first, to avoid an unnecessary deprecation at this time. This is a backport package, so there's an inherent amount of outdatedness expected from its users.

@ryanhiebert
Copy link
Owner

Actually, it looks like it's ultimately Tox that's dropped py33 support, so I think I'll likely need to pin that to an older version.

@mpeteuil
Copy link
Contributor Author

Thanks @ryanhiebert, sounds good.

I started digging into the source of failure over the weekend but got pulled away. Here are related resources that I found that you may have already known about or come across.

Tox dropped support in late 2017 in this PR tox-dev/tox#689 like you mentioned. Setuptools appears to be supporting it for just a little longer, but is no longer running builds against it in travis (pypa/setuptools#1372). Pytest also dropped 3.3 support in November 2017 (not related to this since unittest is used in this project). In general it seems 3.3 support is getting harder to find since it's end of life in September 2017.

@ryanhiebert
Copy link
Owner

Hmm. I might have to just drop Tox for this project, given my desire to keep running on these old versions, since that's the target audience. I don't use this project anymore, because I've personally gotten onto Python 3 everywhere, but anyone needing this package is more likely to need support for some older and unsupported version. I'll see about just using vanilla Travis for testing now, if it's not too much inconvenience to do it. A PR is welcome, though I do wonder if I'll need to do something manually in order to get the release stage for Travis working (since I won't have the Tox-Travis deprecated "after" feature to use anymore).

@ryanhiebert ryanhiebert merged commit 01b6014 into ryanhiebert:master May 22, 2018
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