Remove unused variables #1007
Remove unused variables #1007
Conversation
| $location_sanitized_tmp = regsubst($location, '\/', '_', 'G') | ||
| $location_sanitized = regsubst($location_sanitized_tmp, '\\\\', '_', 'G') | ||
| } else { | ||
| $location_sanitized = $location_sanitized_tmp |
There was a problem hiding this comment.
Shouldn't the right side be $localization? The current bar there will be uninitialized at this point.
There was a problem hiding this comment.
Yes you are correct. Should be $location not $location_sanitized_tmp
There was a problem hiding this comment.
I agree with the proposed changes; that said, maybe I'm being dumb, but can someone point to me where $location_sanitized is actually being used in current version of the module? The template only references $location, and a recursive grep doesn't turn up other instances of $location_sanitized. Also, I don't think locations would work if we actually turned slashes into underscores even in locations that aren't regexes, so unless it were used for something less direct (like naming fragments), I'm not sure that this code is really doing something that would cause a problem.... Is it the latest version of the module you're seeing this with?
Also, don't these proposed changes do the munging if it is a regex, though (if !($location =~ /^~/) {), or am I reading that wrong?
Lastly, not a big deal, but if it's not too much of a hassle to change, I'm trying to switch to what @hunner said is the new convention instead of things like $foo_real or $foo_tmp, which is
e.g., (in server.pp: $_ssl_redirect_port = $ssl_redirect_port). Or maybe just do both substitutions in one line? That seems a little tidier, but maybe too hard to read?
I think having good tests for various scenarios (regex / non regex locations with / without forward and backslash characters) will definitely help make sure we don't miss something or introduce new problems.
There was a problem hiding this comment.
I think you are correct. location_sanitized does not seem to be in use anymore. See 15dfc77 for the change that removed the use of that variable. Now it just takes an md5 of $location to build up the title of the fragment.
I'll modify this pull request to remove those lines.
You are also correct that I'm actually on 0.2.7 (bad me for trying to be sneaky) the above commit went out with the 0.3.0 release.
There was a problem hiding this comment.
Good news is all I should have to do to fix my issue is upgrade the module :)
|
I'll work on getting tests written |
mattkenn4545
changed the title
|
@mattkenn4545 Thanks! If possible, can you squash the commits (or suppress the earlier one)? |
|
Sure thing. |
…x-location Remove unused variables
See PUP-2612