Allow groups option to be an array so that it allows spaces in field names #479

Closed
wants to merge 1 commit into
from

3 participants

@IamDavidovich

Field names can contain spaces (as they are a CDATA type). This causes problems with groups, as the string listing the fields is split on spaces.

This commit allows the groups option to be either a space-separated string or an array of strings so that field names with spaces can be added as an array.

@Jelle-S

We bumped in to this issue at the Clientside Validation Drupal module. See Issue #1705112: Checkboxes validation

@jzaefferer
Owner

Thanks for the contribution. Could you add a unit test for this?

@IamDavidovich

No problem. Thanks for a great project :)

I've added a unit test around the processing of string and array group settings. It is specifically limited to testing the change I added, rather than a full smoke-test style of checking validation etc. I couldn't see any specific groups testing, so I could look at adding that too if you want.

@jzaefferer
Owner

Yes, adding general tests for the feature would be useful. While doing that, can you also run grunt lint and make lil' Travis happy? See also https://travis-ci.org/jzaefferer/jquery-validation/builds/3373464

@IamDavidovich

Sorry about the spam. I've fixed the problems that Travis was complaining about.

I also found the "group error messages" test, which pretty much covered most of what I was going to confirm in the other test. I can put some extra depth in to those at some point, but that seems to cover it for now.

Lastly, I'm feeling that I should close this pull request, squash all my commits and roll a new one. I assume that would be preferable to the spammy commits above?

@jzaefferer
Owner

You can squash commits and force-push to update the PR, no need for a new one. I recommend an interactive rebase, e.g. git rebase upstream/master -i. Makes it easy to select which commits to keep, update or remove. This change should be just one commit. Useful technique, though if you have problems, I can also take care of it.

@IamDavidovich IamDavidovich Allow groups to be an array or a string - Issue #479
Field names can contain spaces (as they are a CDATA type). This causes problems with groups, as the string listing the fields is split on spaces.

This commit allows the groups option to be either a space-separated string or an array of strings so that field names with spaces can be added. It also adds a unit test to ensure that both string and array based groups are processed into the same data structures.
aca8e66
@IamDavidovich

Thanks! I'm familiar with rebasing, I just assumed that forcing the push would play havoc with the pull request.

Let me know if there's anything else you want in this commit.

@jzaefferer jzaefferer added a commit that closed this pull request Dec 10, 2012
@IamDavidovich IamDavidovich Allow groups to be an array or a string - Fixes #479
Field names can contain spaces (as they are a CDATA type). This causes problems with groups, as the string listing the fields is split on spaces.

This commit allows the groups option to be either a space-separated string or an array of strings so that field names with spaces can be added. It also adds a unit test to ensure that both string and array based groups are processed into the same data structures.
b82d8cc
@jzaefferer
Owner

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment