Merge space() into indent() for indentation handling (#91)#91
Merge space() into indent() for indentation handling (#91)#91shreyasutha wants to merge 3 commits intoworldbank:devfrom
Conversation
luisesanmartin
left a comment
There was a problem hiding this comment.
@shreyasutha thanks a lot for working on this. After reviewing the code, I don't think we should apply any changes in it but only in the documentation because of these reasons:
spacealready defaults to the value inindentwhen the user doesn't define it. Hence, if in the future no user ever usesspaceagain, the command would still work as intended- Keeping this differentiation between
spaceandindentis critical for back-compatibility. The current solution you added to this commit is not back-compatible, as it would use the same value (indentval, the value inindent) even if the user defines two different values forspaceandindent - Then, I think the best solution for back-compatibility + merging both options is just modify the documentation to:
- Delete any mention of
space - Explain that
indentchecks the spaces for indentation and replaces hard-tabs with that number of spaces
- Delete any mention of
That way, any old users still using space and indent will be able to use it with the original functionality, while new users will only use indent which will also be assigned internally to the value for space.
Can you then modify the documentation in this PR and revert your changes to the code? I know you were looking for a coding challenge here and I'm sorry this became only a documentation challenge. However, feel free to take on any other issues on repkit or lint if you can!
| NOSUMmary /// | ||
| Indent(string) /// | ||
| Linemax(string) /// | ||
| Space(string) /// |
There was a problem hiding this comment.
these lines define the syntax of the command. As we want to keep the option space for backwards compatibility, we shouldn't remove it from the syntax definition
src/ado/lint.ado
Outdated
| if ("`indent'" != "") { | ||
| local indentval = "`indent'" | ||
| } | ||
| else if ("`space'" != "") { | ||
| local indentval = "`space'" | ||
| } | ||
| else { | ||
| local indentval = 4 | ||
| } |
There was a problem hiding this comment.
the condition in 39 will never activate, see that line 35 already already assigns a value for indent (4) if there isn't one already.
After checking the code again, I'm actually not convinced we should change any of this code as space will always take the value in indent when it's not defined by the user. See my other general comment on the review about changing only the documentation for a more detailed explanation.
|
Hi @luisesanmartin, thanks again for the helpful review! I’ve now:
Let me know if any further tweaks are needed, but I believe this version aligns with your suggestion to treat this as a documentation-only update. |
This edit merges the indent() and space() options into a single interface. It: