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

schema["type"] should allow str or unicode, and the error now prints the type #96

Closed
wants to merge 4 commits into from

Conversation

EliteMasterEric
Copy link
Contributor

A fix for a frustrating bug I encountered. I'm surprised the unit tests even work.

@Grokzen
Copy link
Owner

Grokzen commented Apr 15, 2017

@mastereric You broke the tests, needs to be fixed and the linting of the changed code is not good. Both needs to be fixed before i could accept this PR.

@@ -380,9 +380,9 @@ def init(self, schema, path):
t = DEFAULT_TYPE
self.type = t
else:
if not isinstance(schema["type"], str):
if not (isinstance(schema["type"], str) or isinstance(schema["type"], unicode)):
Copy link
Owner

@Grokzen Grokzen Apr 16, 2017

Choose a reason for hiding this comment

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

You should glob them together into into something like

if not isinstance(schema["type"], (str, int))

and get the same logic and much cleaner code.

raise RuleError(
msg=u"Key 'type' in schema rule is not a string type",
msg=u"Key 'type' in schema rule is not a string type (found "+str(type(schema["type"]))+")",
Copy link
Owner

Choose a reason for hiding this comment

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

Horrible python linting, should be

msg=u"Key 'type' in schema rule is not a string type (found " + str(type(schema["type"])) + ")",

@EliteMasterEric
Copy link
Contributor Author

Apologies for the delay in fixing your issues.

I corrected the linting on my code changes, and discovered that modifying one of the error messages to be more verbose broke an assertion in one of the unit tests, which I fixed. All unit tests now pass on my machine.

@coveralls
Copy link

coveralls commented Aug 25, 2017

Coverage Status

Coverage remained the same at 87.2% when pulling 9443f00 on MasterEric:unstable into e7fac6a on Grokzen:unstable.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 87.2% when pulling 9443f00 on MasterEric:unstable into e7fac6a on Grokzen:unstable.

@EliteMasterEric
Copy link
Contributor Author

@Grokzen Looking at the Travis page, it says that 2 checks failed, however there are no logs for the checks that failed. All tests where there are logs show all checks have passed. Can you please fix this issue?

@Grokzen Grokzen closed this Dec 3, 2017
@EliteMasterEric
Copy link
Contributor Author

@Grokzen This issue appears to have been closed without comment, can you please clarify?

@Grokzen
Copy link
Owner

Grokzen commented Feb 6, 2018

@mastereric I accidentally closed all open MR:s when i removed the destination branch. I am currently manually merging all of them including this one.

@Grokzen
Copy link
Owner

Grokzen commented Feb 6, 2018

@mastereric Manually merged the commits here 24adf3d..dfb45cb

@Grokzen Grokzen added the Merged label Feb 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants