Skip to content

Allow min_length==0 for Column.Text fields#722

Merged
aboudreault merged 1 commit into
apache:masterfrom
HaraldNordgren:master
Oct 2, 2017
Merged

Allow min_length==0 for Column.Text fields#722
aboudreault merged 1 commit into
apache:masterfrom
HaraldNordgren:master

Conversation

@HaraldNordgren
Copy link
Copy Markdown

@HaraldNordgren HaraldNordgren commented Mar 24, 2017

Current behaviour treats 0 as not specified at all. If this is not on purpose we should check for None explicitly instead to allow us to store empty strings in the database.

@datastax-bot
Copy link
Copy Markdown

Hi @HaraldNordgren, thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes.

Sincerely,
DataStax Bot.

@mambocab
Copy link
Copy Markdown
Contributor

Thanks for the PR! I'd like to add a test or two, but after that, I can merge after you've signed the CLA. I've created a JIRA ticket and marked it for inclusion in 3.9, our next release.

@mambocab
Copy link
Copy Markdown
Contributor

Sorry, I've jumped the gun in my reply. We've had some discussion about how to address this and are discussing other possible APIs for this solution. We may not get to this before our next release.

Still, please sign the CLA so if we decide to implement this solution, we can merge quickly. Thanks!

@datastax-bot
Copy link
Copy Markdown

Thank you @HaraldNordgren for signing the Contribution License Agreement.

Cheers,
DataStax Bot.

@HaraldNordgren
Copy link
Copy Markdown
Author

Could someone merge or close this one?

Comment thread cassandra/cqlengine/columns.py Outdated
"""
self.min_length = (
1 if not min_length and kwargs.get('required', False)
1 if min_length is not None and kwargs.get('required', False)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it should be 1 if min_length is None and kwargs.get('required', False)

Copy link
Copy Markdown
Contributor

@beltran beltran left a comment

Choose a reason for hiding this comment

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

@HaraldNordgren wdyt about my previous comment?

@HaraldNordgren
Copy link
Copy Markdown
Author

@bjmb Yes, I think your comment probably makes sense.

I haven't worked with this code in a long while, but looking at it now I'm guessing that was a simple mistake on my part.

@HaraldNordgren
Copy link
Copy Markdown
Author

@mambocab I like tests, but I'm not quite sure where to begin with this one.

Setting self.min_length = 10000 and running tox proves to me that this logic is already untested as it succeeds without any problems.

@aboudreault aboudreault merged commit 227af86 into apache:master Oct 2, 2017
@aboudreault
Copy link
Copy Markdown
Contributor

Thanks. Added a minimal value test here: 32037f3

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.

5 participants