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

check the length of lists given in options before writing them #48

Merged
merged 3 commits into from
Mar 29, 2017

Conversation

yomimono
Copy link
Contributor

/cc @haesbaert - I don't think this is too gory, what do you think?

@yomimono
Copy link
Contributor Author

I should make this parameter optional since there's only one call where it's anything other than 1, standby for force push...

@haesbaert
Copy link
Member

haesbaert commented Mar 29, 2017

Hmm I think that's doable, but we really need to check them all before default to min=1.

Can you split the bugfixes/whitespace fixes into a different commit, that wrong code number is a good find.

If after we check them all, it might be that the option could be something like emptyok=true instead of giving a min_len, I suspect we will only use min_len=0.

Since we are here, we might consider minimum length checks on single options too, like strings

Hah, only now I saw that the inverse is already done in options_of_buf.

@yomimono
Copy link
Contributor Author

yomimono commented Mar 29, 2017 via email

@haesbaert
Copy link
Member

I think the diff is fine, we're already enforcing min_len for every list option on the other way. So we should enforce when encoding too.

Just split them up and I'll merge :D.

Thanks for this !

@haesbaert
Copy link
Member

This actually prompted me to have a look at get_8_list and get_16_list, they don't enforce min_len, I'll write a diff.

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.

2 participants