Fix assignment, variable name can be alpha numeric#46
Fix assignment, variable name can be alpha numeric#46sereysethy wants to merge 0 commit intoidank:masterfrom
Conversation
|
Thanks for the change. This function is based on http://git.savannah.gnu.org/cgit/bash.git/tree/general.c?id=df2c55de9c87c2ee8904280d26e80f5c48dd6434#n263 Can you make sure the change you're making is correct with respect to how Bash handles this? Also, can you please add a test? Thanks! |
|
I verified in the bash source code, here it is the comment that I found: So variable name can be letters, digits and |
|
I think this will also accept variable names that start with a digit, no? |
|
No, you cannot have variables starting with a digit, this will have a conflict with the reserved parameters (positional parameters) passed to a script or a function |
|
Doesn't your change allow that? |
idank
left a comment
idank
left a comment
There was a problem hiding this comment.
Can you please squash the commits? I think one commit is enough here.
tests/test-tokenizer.py
Outdated
| t(tt.WORD, "'b '", [2, 7], set([flags.word.QUOTED])), | ||
| t(tt.WORD, 'c', [8, 9])]) | ||
|
|
||
| def test_variables(self): |
There was a problem hiding this comment.
please add a test for the invalid case where you a variable name starts with a digit
There was a problem hiding this comment.
How to write an invalid case?
There was a problem hiding this comment.
try to parse something like 0var=1
There was a problem hiding this comment.
Can you elaborate more? Parse in the test case?
There was a problem hiding this comment.
Yes, please add a test case for the problematic assignment which shows that parsing fails. If you're having trouble, I can do it.
There was a problem hiding this comment.
I added a failed case, but I am not sure if it is a correct way to do it.
bashlex/tokenizer.py
Outdated
| c = value[0] | ||
|
|
||
| def legalvariablestarter(x): | ||
| return x.isalpha() |
There was a problem hiding this comment.
missing or x == '_' in here since _x is a legal variable name. please add a test.
2 participants
No description provided.