LUN-2235 Clean snippet variable names #62
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Valid characters for django template variables are [a-zA-Z0-9_]. Snippets that have invalid variables cannot use those variables, but creating a migration that would change them can break the snippet templates in subtle ways.
The fake admin site is registered because the smartsnippets.admin module requires it when loaded.
| @@ -155,6 +160,29 @@ def test_vars_generated(self): | |||
| self.assertEqual(Variable.objects.count(), 3) | |||
| self.assertEqual(plugin3.variables.count(), 1) | |||
|
|
|||
| def test_validation_passes_for_correct_variables(self): | |||
There was a problem hiding this comment.
Can you add some tests with valid variable names? Just to make sure that the base functionality is not broken.
| @@ -12,6 +13,10 @@ | |||
| SmartSnippetPointer, | |||
| Variable, | |||
| ) | |||
| class FakeSiteAdmin(admin.ModelAdmin): | |||
There was a problem hiding this comment.
Why is this needed?
Maybe add a short comment explaining why, as it's not very intuitive.
There was a problem hiding this comment.
I've added the explanation in the commit, but maybe it would be better to describe it in the code.
| def validate_unique_variable_names(self): | ||
| """ Validates name uniqueness over all variable inlines. """ | ||
| unique_variable_names = set() | ||
| duplicate_variable_names = set() |
There was a problem hiding this comment.
(Optional) Fancier way of doing the same thing:
duplicate_variable_names = [
var_name
for var_name, count in collections.Counter(['foo', 'bar', 'foo']).iteritems()
if count > 1
]
There was a problem hiding this comment.
Thanks. It's a little bit more clear this way.
laurafeier
pushed a commit
that referenced
this pull request
Jul 29, 2015
LUN-2235 Clean snippet variable names
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.