Skip to content
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

linting document update #13

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from
Draft

linting document update #13

wants to merge 3 commits into from

Conversation

ryandanehy
Copy link
Collaborator

This is my first draft of update linting docs. Hopefully this will spur discussion about how we want to to move forward.

@ryandanehy ryandanehy requested a review from pelesh May 16, 2023 20:56
Copy link
Collaborator

@pelesh pelesh left a comment

Choose a reason for hiding this comment

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

I added some suggestions. I am not sure how flexible is clang-format and how much freedom we have to adjust formatting.

Comment on lines +85 to +98
};
```

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
};
```
};
} // namespace N

Comment on lines +98 to +155
-enum ShootingHand { SH_Left, SH_Right }; // use upper-case initials as prefix
+enum ShootingHand
{
SH_Left,
SH_Right
}; // use upper-case initials as prefix
Copy link
Collaborator

Choose a reason for hiding this comment

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

enum, if it is short, should be in the same line. I would not make this change.

CONTRIBUTING.md Outdated
Comment on lines 267 to 269
else {
value -= 1;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider following:

Suggested change
else {
value -= 1;
}
else
{
value -= 1;
}

CONTRIBUTING.md Outdated
Comment on lines 258 to 260
### Position of `else` keyword

Keyword `else` should follow the if closing brace.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
### Position of `else` keyword
Keyword `else` should follow the if closing brace.

Can get rid of "position of else" as it is not relevant in Allman case.

Comment on lines +123 to +180
-Cowboy::Cowboy()
- : Age(45), GunCount(2) {
- std::cout << "I am alive!" << std::endl;
-}
+Cowboy::Cowboy() : Age(45), GunCount(2) { std::cout << "I am alive!" << std::endl; }
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we configure lint to enforce something like this:

Suggested change
-Cowboy::Cowboy()
- : Age(45), GunCount(2) {
- std::cout << "I am alive!" << std::endl;
-}
+Cowboy::Cowboy() : Age(45), GunCount(2) { std::cout << "I am alive!" << std::endl; }
```
Cowboy::Cowboy()
: Age(45),
GunCount(2)
{
std::cout << "I am alive!" << std::endl;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know off the top of my head, but there is alot flexibility with clang-format so let me look around.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are all great recommendations, and I will see to them getting applied both in document and actual application of clang format.

@ryandanehy ryandanehy marked this pull request as ready for review June 1, 2023 21:01
@ryandanehy
Copy link
Collaborator Author

I think this is in a good state. @pelesh would love your opinion.

@pelesh pelesh marked this pull request as draft June 29, 2023 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants