(CONT-551) Fix get_exec_titles bug #36
Conversation
GSPatton
force-pushed
the
CONT-551-Fix_get_exec_titles_bug
branch
from
676caa9 to
6e304d6
Compare
| @@ -117,4 +117,13 @@ def get_exec_titles | |||
| def interpolated?(token) | |||
| INTERPOLATED_STRINGS.include?(token.type) | |||
| end | |||
|
|
|||
| # Finds the index of the next instance of `value` in `array` from the `current_index` | |||
| def next_instance_index(array, current_index, value) | |||
There was a problem hiding this comment.
Really small nit but we should consider our parameter names.
If i read the function out of context i'm asking what is array - does that mean I can pass any array to it and it will do what I want?
Maybe this should be tokens?
Also, would calling tokens on L104 would return all of the tokens that puppet-lint knows about? If that is the case could we give it the current token and work forward from there?
I'm not 100% on the comment above.. more or a discussion point than anything else.
There was a problem hiding this comment.
i thought to keep the function agnostic to what array is being passed for reusability... thinking about it now, reusability isn't actually necessary here
There was a problem hiding this comment.
yeah i agree. Tokens would be more descriptive
There was a problem hiding this comment.
about L104... do I not need to pass the full array so that i can use array.length anf array[i].value etc??
There was a problem hiding this comment.
I can't say for sure without playing with it but I had a thought that your search would happen from position 0 and could therefore return a false value..
I wonder if we could pass a slice starting from the current index.
There was a problem hiding this comment.
tried passing a slice but this introduced some complications
ie we are iterating through indices in this new tokens array slice and so the index of the ':' found in next_instance_index won't be the same index as in the full tokens array
There was a problem hiding this comment.
if you don't think passing the full tokens array impacts the speed of the check too much, i'd rather stick with passing the full tokens array
GSPatton
force-pushed
the
CONT-551-Fix_get_exec_titles_bug
branch
2 times, most recently
from
9df4c4f to
d61ecf4
Compare
GSPatton
force-pushed
the
CONT-551-Fix_get_exec_titles_bug
branch
from
d61ecf4 to
87cd885
Compare
Prior to this commit, get_exec_titles (which acts as a replacement to puppet-lint's get_titles function) had a bug where, often, the title string of an exec resource was obtained incorrectly. There were multiple examples where the resulting hash of title tokens would contain much more tokens than were actually in the title - i.e. many tokens after the semi colon concluding the title. This resulted in many identical warnings in the output of the lint check for one interpolated variable. The source of this bug was in how the index in the tokens array for the start and end of the title were retrieved. This commit fixes this by removing the use of rindex method which returns the index of the last object in the array. Returning the last object in the array (tokens) is not what should be done here. Instead we want to find the NEXT INSTANCE of objects. The start of the title is next_code_token.next_code_token after an exec and the end of the title is the NEXT INSTANCE in the tokens array of ':'.
GSPatton
force-pushed
the
CONT-551-Fix_get_exec_titles_bug
branch
from
87cd885 to
76bf8d0
Compare
Prior to this commit, get_exec_titles (which acts as a replacement to puppet-lint's get_titles function) had a bug where, often, the title string of an exec resource was obtained incorrectly. There were multiple examples where the resulting hash of title tokens would contain much more tokens than were actually in the title - i.e. many tokens after the semi colon concluding the title. This resulted in many identical warnings in the output of the lint check for one interpolated variable.
The source of this bug was in how the index in the tokens array for the start and end of the title were retrieved. This commit fixes this by removing the use of rindex method which returns the index of the last object in the array. Returning the last object in the array (tokens) is not what should be done here. Instead we want to find the NEXT INSTANCE of objects. The start of the title is next_code_token.next_code_token after an exec and the end of the title is the NEXT INSTANCE in the tokens array of ':'.