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

Type convert boolean variables correctly (closes #149) #150

Merged
merged 6 commits into from Jul 11, 2018

Conversation

tommilligan
Copy link
Contributor

@tommilligan tommilligan commented Jul 10, 2018

See #149 for issue details.

This PR:

  • adds failing tests that show !bool type conversion is incorrect
  • implements a fix for !bool using a custom bool constructor class

The custom bool constructor is similar to the builtin bool constructor as per PEP 285, and as discussed here

@michaelboulton
Copy link
Member

Rather than using a custom constructor it is probably better to use strtobool from the standard library, which can handle a lot more than just True and False (including all the weird yaml ones like on and y)

Other than that, this all looks good!

@tommilligan
Copy link
Contributor Author

Happy to use strtobool instead, if it's okay to allow a wider range of values. Agree it's better than a custom implementation.

See the new commit for changes:

  • it still has to be wrapped in a constructor __new__ method to match other constructors
  • strtobool actually returns 0|1, so it's wrapped in bool

@tommilligan
Copy link
Contributor Author

There is a long running known issue with combining distutils, pylint and virtualenv (which tox uses internally). pylint reports a false positive that distutils cannot be found.

I have added distutils as an ignored module in .pylintrc.

@michaelboulton michaelboulton merged commit fbf76e2 into taverntesting:master Jul 11, 2018
@michaelboulton
Copy link
Member

Thanks for that, it should be deployed in version 0.15.1.

@tommilligan
Copy link
Contributor Author

@michaelboulton PyPI latest version appears to be 0.14.4 - have had a quick look at the Travis logs and can't see any obvious errors

@michaelboulton
Copy link
Member

It seems like the deplyment via travis isn't actually deploying - I don't have time to look at this until monday though unfortunately,

I might just change it so it's using gitlab CI to do the deployment

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

3 participants