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

Create more verbose error message for unicode strings as formulas #56

Closed
wants to merge 1 commit into from

Conversation

thorstenkranz
Copy link

Instead of supporting unicode, give a useful error message when a formula is unicode.

@@ -56,6 +57,9 @@ def _try_incr_builders(formula_like, data_iter_maker, eval_env,
data_iter_maker,
NA_action)
else:
# check if formula_like is another string type (which might be unsupported)
if isinstance(formula_like, string_types):
raise PatsyError("Unsupported string type for formula (e.g., unicode in Python 2.X).")
Copy link
Member

Choose a reason for hiding this comment

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

I think this is written in a little confusing way -- the only posible cases are:

  • Python 2: string_types is equivalent to (str, unicode). str is handled above, so only unicode can hit this point.
  • Python 3: string_types is equivalent to str. str is handled above, so this point can't be reached.
    There aren't any other string types floating around.

So I'd probably write the check like if not six.PY3 and isinstance(formula_like, unicode): ..., and then the error message could be more specific. (Unfortunately you can't use if six.PY2 ... because apparently older versions of six define PY3 but not PY2... I got another bug report about this at some point.)

@njsmith
Copy link
Member

njsmith commented Mar 4, 2015

Also, could you add a test for this? We try to maintain ~100% line coverage. Just something simple like assert_raises_regexp(PatsyError, "unicode", dmatrix, u"some formula that used to produce a confusing error", {})

has2k1 added a commit to has2k1/patsy that referenced this pull request Oct 18, 2015
@has2k1 has2k1 mentioned this pull request Oct 18, 2015
@njsmith
Copy link
Member

njsmith commented Oct 27, 2015

Closing in favor of #76.

@njsmith njsmith closed this Oct 27, 2015
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

2 participants