Sanitize source text to avoid mis-treating variable names as Ruby Const #8
Conversation
auto-unit converstions for recom variables CO2f and pCO2s are added. The negative sign used in "mmolC/m2/d" -> "kg m-2 s-1" is to correct the sign change for into ocean or into atm.
| src_txt = new_txt.join("\n") | ||
| end | ||
| src_txt | ||
| end |
There was a problem hiding this comment.
Is the indentation correct in this case?
There was a problem hiding this comment.
Yep, indentation needs to be addressed here.
There was a problem hiding this comment.
@siligam: If you aren't aware of this feature yet; GitHub reviewing offers "change suggestions" which you can do by pushing the little button with a page and + - symbols, or directly in markdown with a code-fenced tagged with "suggestion" and then using diff style syntax with plus and minus. As a fence it looks like this:
```suggestion
+ add this line
- remove this one
```
Docs are here under step #7.
If you already knew that, emm...sorry for the spam :)
| instance_eval(src_txt, src_txt) | ||
| @eval_mode = false | ||
| end | ||
|
|
||
| def sanitize(src_txt) |
There was a problem hiding this comment.
I think is in general a better practice to have a more descriptive name of the function, for example handle_uppercase_vars. In this way when you are reading the code in line 18 you already have an idea of what this function is doing without having to look at the function itself
There was a problem hiding this comment.
Plus 1 to this comment 😄 (But take with a grain of salt: I am a little bit neurotic about naming things)
There was a problem hiding this comment.
Sure, that make sense to be more precise about what this function does by giving it a proper name. This was not a priority thing than get the logic right at that time but thanks for reading through and suggesting a proper name.
I still need to modify the logic a little bit in deciding where to insert the bug fix line. In the current implementation it is introduced just a line above where it find a Capitalised variable line but I want to this to move to the top of the file in case some other function depends on the same issue being fixed.
Potential solution to address issue #7 is to sanitise the variable name from user input source text. The variable name is kept as is but and extra line is added initialising the variable as a string with the same variable name. For instance, the following line produces an NameError (mis-treating
CO2f_dayas Ruby Constant as the name starts with a capital letter)cmorize CO2f_day => [fgco2_Omon]To overcome this, the following extra line is added:
The sanitisation process does the exact same thing at runtime so users do not have to explicitly provide this extra line.