Skip to content

Conversation

IamDavidovich
Copy link
Contributor

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
Copy link

Jelle-S commented Aug 17, 2012

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

@jzaefferer
Copy link
Collaborator

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

@IamDavidovich
Copy link
Contributor Author

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
Copy link
Collaborator

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

@jzaefferer
Copy link
Collaborator

@IamDavidovich
Copy link
Contributor Author

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
Copy link
Collaborator

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.

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.
@IamDavidovich
Copy link
Contributor Author

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
Copy link
Collaborator

Thanks!

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.

3 participants