-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DOC: Add guideline on using a backslash as line continuation character #190
base: main
Are you sure you want to change the base?
DOC: Add guideline on using a backslash as line continuation character #190
Conversation
Follow-up to ITK pull request InsightSoftwareConsortium/ITK#3713 commit InsightSoftwareConsortium/ITK@69151c3 "STYLE: Remove backslash + indent from literals, to avoid unwanted spaces"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure that I agree with the terms.
// Wil we go to sleep or not? \ | ||
Sleep(); | ||
\end{minted} | ||
\normalsize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although as the description says one may easily think that the above method will be called, I am not convinced why we should clarify this: this is not about ITK's style, but rather a C++/programming language feature. I would not even mention this. Has the situation been found in the recent PRs to ITK removing backslashes in the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your feedback. My intention was to show two different cases for which the use of backslashes is problematic. In order to reach the conclusion that we discourage them in general (except for in macro definitions), in ITK C++ source code.
However, we could also just have a guideline specifically against backslashes in string literals. Without mentioning other possible cases. Is that what you prefer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although in other places the "recommended", "suggested", etc. words have been used (and this has also caused some debate), IMHO, in this case, as I said, a commit containing backslashes is unlikely to get accepted following your PRs in ITK, so to me, following your PRs and the improvement in style/message rendering they bring, "ITK does not use backslashes to break continuous string literals into multiple lines; it relies on its format enforcing tool -name it and cross ref- to break them." (or something similar with the same message; and then the macro stuff would come.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your comments Jon, but...
a commit containing backslashes is unlikely to get accepted following your PRs in ITK
If that's the case, we might as well abandon this PR, right?
I thought it would be helpful to make it a guideline, because I encountered quite a few of those backslashes before doing ITK PR InsightSoftwareConsortium/ITK#3713. But if we may assume that those backslashes would not be accepted anymore now anyway, the guideline may not be necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you feel it Niels.
\end{minted} | ||
\normalsize | ||
|
||
Moreover, the use of backslashes in a multiline string literal also appears |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjust the "Moreover (...) also" if we agree that the above content should be removed.
|
||
Moreover, the use of backslashes in a multiline string literal also appears | ||
troublesome. For example, the following \code{badDescription} has all the space | ||
characters of the indentation between "developers with " and "an extensive suite" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LaTeX uses double backticks (open) + simple ticks (close) to render the quotes/literals.
characters of the indentation between "developers with " and "an extensive suite" | ||
embedded inside the string, which may not be intended. The definition of | ||
\code{goodDescription} shows how to define such a lengthy string properly (as | ||
the compiler concatenates adjacent string literals automatically). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm not mistaken, the literal might be automatically formatted/changed by the ITK style rules. So to me best would be to write the literal in a single line and let the code formatter do its job. If we agree on this, the above should be re-written .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jhlegarreta Would you like to rewrite the text or make another proposal yourself? Please feel free to make an alternative PR, if you want to, no problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine if you amend the current commit following the comments.
discouraged. Exception: a lengthy preprocessor macro definitions may actually | ||
be more readable when it spans multiple lines, so in this case, the use of | ||
backslashes is OK. But even then, such line continuation characters should not | ||
appear inside a string literal. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So depending on my above comment, we should never use the backslash. It is not only discouraged: any PR that has one is likely to get a comment on it requesting to be changed.
I would put an example or cross-reference the appropriate section (if it exists, or maybe create one if it doesn't) concerning the marcos. Have not reviewed our definitions, but maybe we use the backslash always? Not sure what the rule has been/a pattern exists.
I comment on the style: I would rewrite the second sentence so that we integrated "Exception" into the sentence and would change "OK" for e.g. "accepted"/", ecommended".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backslashes are routinely used in macros, e.g. itkSimpleNewMacro.
Follow-up to ITK pull request InsightSoftwareConsortium/ITK#3713 commit InsightSoftwareConsortium/ITK@69151c3
"STYLE: Remove backslash + indent from literals, to avoid unwanted spaces"