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

Accept a num_elements_provider in FieldArray to specify a size in elements instead of bytes. #35

Merged
merged 3 commits into from
Jul 27, 2016

Conversation

rcfox
Copy link
Contributor

@rcfox rcfox commented Jan 11, 2016

After some thought, I was able to come up with a way to resolve #34 myself.

Ryan Fox added 2 commits January 11, 2016 17:05
@rcfox
Copy link
Contributor Author

rcfox commented Jan 11, 2016

Not sure what happened with the coverage build, but I don't think it's my fault.

coverage runtests: commands[1] | coveralls
Traceback (most recent call last):
  File ".tox/coverage/bin/coveralls", line 11, in <module>
    sys.exit(wear())
  File "/home/travis/build/digidotcom/python-suitcase/.tox/coverage/lib/python3.5/site-packages/coveralls/__init__.py", line 79, in wear
    from coveralls.control import coveralls
  File "/home/travis/build/digidotcom/python-suitcase/.tox/coverage/lib/python3.5/site-packages/coveralls/control.py", line 1, in <module>
    from coverage.control import Coverage
ImportError: cannot import name 'Coverage'
ERROR: InvocationError: '/home/travis/build/digidotcom/python-suitcase/.tox/coverage/bin/coveralls'

break

if self.num_elements is not None and len(self._value) != self.num_elements:
raise SuitcaseParseError("Expected %r elements but received %r." %
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend using %s and %d, respectively, in this format string. Using %r will add single quotes which could be confusing.

Edit: Ignore.

@posborne
Copy link
Contributor

I actually prefer %r in this case as %s formatting will not show some characters (binary non-printable). In addition, things like newlines will just be included and output when printed which can be very confusing.

@posborne
Copy link
Contributor

@rcfox I can look at the coverage stuff and will look at this PR in the next few days (hopefully) once I get back from traveling.

@mikewadsten
Copy link
Contributor

@posborne You're absolutely right on the %r stuff. I had it in my head that "%r %r" % (None, 1) yields the string 'None' '1', when that's obviously not the case.

@posborne
Copy link
Contributor

Build failure should be fixed by #41 and I think this is a pretty valuable change to have in. I'm going to go ahead and include it.

Thanks and sorry for the long delay!

@posborne posborne merged commit 8c7d1dd into digidotcom:master Jul 27, 2016
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.

Accept a "number of elements" argument for FieldArray
3 participants