PYTHON-609: Align Text and Ascii columns behaviour #623
Conversation
|
Ah. Just realized that there's two sets of tests: |
|
Unfortunately our integration tests are a little too hefty to run on travis. We have some separate internal infrastructure to run these. It is expected. |
kdeldycke
changed the title
|
Both unit and integration tests pass on my machine. This PR is ready to be reviewed and merged. |
|
As a side note, this covers old cqlengine/cqlengine#283 PR |
|
We will use this PR, instead of mine. I like the way you handled the validation better. For reference the jira number covering this in our project is |
|
OK, as you wish. Do not hesitate to tell me of this PR needs some changes or improvements. For instance, do you want me to separate the string handling logic in its own class? Like you did with the |
| raise ValidationError('{0} is shorter than {1} characters'.format(self.column_name, self.min_length)) | ||
| return value | ||
|
|
||
|
|
||
| class Ascii(Text): | ||
| """ |
There was a problem hiding this comment.
We either need a separate class extending the init and validation logic, or Text needs to extend Ascii, either way is fine.
There was a problem hiding this comment.
The reason I was thinking the other way around is that I'm about to add a validation() in Ascii to extend on Text's, and add stricter checks on legal ASCII character range. Do you want me to implement this?
There was a problem hiding this comment.
Just added the extra validation on ASCII characters in commit 1a72f87 .
|
We were also wondering what the priority level of this change is for you? Is it blocking a critical path for you? |
|
This is not highly critical. But once this PR is merged upstream I'll be able to delete some hard-coded checks in my proprietary code, to rely exclusively on CQLengine! 😅 |
|
I addressed all comments on this PR. If you have nothing to add, this is ready for a merge. |
kdeldycke
changed the title
|
Thank you so much for your patience and assistance. Sorry it took me so long to get back to you. This will be included in our next release. 👍 |
|
Thanks @GregBestland for the merge! Really happy to contribute! :) |
This PR:
Asciicolumns.Textcolumns unit-tests for minimal and maximal length.Nonevalue strings handling, especially in relation to therequiredcolumn parameter.Asciicolumn behaviour toText, by addingmin_lengthandmax_lengthparameter support.