Skip to content

cpplint: Disable whitespace/todo#13763

Merged
jwnimmer-tri merged 1 commit intoRobotLocomotion:masterfrom
jwnimmer-tri:whitespace-todo
Jul 31, 2020
Merged

cpplint: Disable whitespace/todo#13763
jwnimmer-tri merged 1 commit intoRobotLocomotion:masterfrom
jwnimmer-tri:whitespace-todo

Conversation

@jwnimmer-tri
Copy link
Copy Markdown
Collaborator

@jwnimmer-tri jwnimmer-tri commented Jul 29, 2020 •
edited by sherm1
Loading

In preparation for enabling C-style comment linting, we disable the whitespace rules for TODOs. They go beyond what GSG demands, and it is unclear how to apply them to C-style comments.

Related to #13762 and RobotLocomotion/styleguide#31.

This change is Reviewable

@jwnimmer-tri
cpplint: Disable whitespace/todo
6012278 
In preparation for enabling C-style comment linting, we disable the
whitespace rules for TODOs.  They go beyond what GSG demands, and
it is unclear how to apply them to C-style comments.
@SeanCurtis-TRI SeanCurtis-TRI self-assigned this Jul 29, 2020
SeanCurtis-TRI
SeanCurtis-TRI reviewed Jul 29, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:LGTM: Since I happened by.
+@SeanCurtis-TRI

Reviewed 1 of 1 files at r1.
Reviewable status: needs at least two assigned reviewers

@jwnimmer-tri jwnimmer-tri marked this pull request as ready for review July 29, 2020 17:03
@rpoyner-tri rpoyner-tri self-assigned this Jul 29, 2020
rpoyner-tri
rpoyner-tri reviewed Jul 29, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: +@rpoyner-tri

Reviewable status: labeled "do not merge" (waiting on @rpoyner-tri)

ggould-tri
ggould-tri reviewed Jul 29, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r1.
Reviewable status: labeled "do not merge" (waiting on @rpoyner-tri)

soonho-tri
soonho-tri reviewed Jul 29, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@soonho-tri soonho-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r1.
Reviewable status: labeled "do not merge" (waiting on @rpoyner-tri)

sherm1
sherm1 reviewed Jul 29, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did this just enforce that there is a single space between the TODO() and the text in TODO(someone) text or is it more subtle? If it's just that why is it a problem in a C-style comment?

Reviewable status: labeled "do not merge" (waiting on @rpoyner-tri)

sherm1
sherm1 reviewed Jul 29, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per f2f: thanks! :lgtm:

Reviewable status: labeled "do not merge" (waiting on @rpoyner-tri)

@jwnimmer-tri
Copy link
Copy Markdown
Collaborator Author

jwnimmer-tri commented Jul 29, 2020 •
edited
Loading

Did this just enforce that there is a single space between the TODO() and the text in TODO(someone) text ...

For the record -- yes, it did that but it also enforced exactly once space at the start // TODO(...) between comment opener and the TODO. That was awkward to try to teach how to deal with C style comments in that way I'm implementing it. In Drake we allow for differing amounts of leading whitespace in the middle of C-style comment blocks.

@jwnimmer-tri
Copy link
Copy Markdown
Collaborator Author

jwnimmer-tri commented Jul 31, 2020

Hearing no objections, I'll merge this now.

@jwnimmer-tri jwnimmer-tri merged commit dc2a939 into RobotLocomotion:master Jul 31, 2020
@jwnimmer-tri jwnimmer-tri deleted the whitespace-todo branch July 31, 2020 13:04
thduynguyen pushed a commit to thduynguyen/drake that referenced this pull request Aug 7, 2020
@jwnimmer-tri
cpplint: Disable whitespace/todo (RobotLocomotion#13763)
1318ace 
In preparation for enabling C-style comment linting, we disable the
whitespace rules for TODOs.  They go beyond what GSG demands, and
it is unclear how to apply them to C-style comments.
thduynguyen pushed a commit to thduynguyen/drake that referenced this pull request Aug 22, 2020
@jwnimmer-tri
cpplint: Disable whitespace/todo (RobotLocomotion#13763)
0cc0eb8 
In preparation for enabling C-style comment linting, we disable the
whitespace rules for TODOs.  They go beyond what GSG demands, and
it is unclear how to apply them to C-style comments.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sherm1 sherm1 sherm1 left review comments

@rpoyner-tri rpoyner-tri rpoyner-tri left review comments

@ggould-tri ggould-tri ggould-tri left review comments

@SeanCurtis-TRI SeanCurtis-TRI SeanCurtis-TRI left review comments

+1 more reviewer

@soonho-tri soonho-tri soonho-tri left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@rpoyner-tri rpoyner-tri

@SeanCurtis-TRI SeanCurtis-TRI

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jwnimmer-tri @sherm1 @rpoyner-tri @ggould-tri @soonho-tri @SeanCurtis-TRI