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

fix indentation and line lenghts in Task.hpp's template #134

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

doudou
Copy link
Contributor

@doudou doudou commented May 15, 2020

No description provided.

@doudou doudou requested review from 2maz and g-arjones May 15, 2020 12:43
* It can be dynamically adapted when the deployment is called with a prefix argument.
short_doc, *long_doc = doc.split("\n")
%>/*! \class <%= task.basename %>
* \brief <%= short_doc %><%= "\n *" + long_doc.join("\n * ") unless long_doc.empty? %>
*/
class <%= task.basename %> : public <%= task.basename %>Base
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please also move the braces to the previous line?

@@ -106,4 +105,3 @@ namespace <%= space %>{
<% end %>

#endif

Copy link
Contributor

Choose a reason for hiding this comment

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

rock_vera requires the final newline

@doudou
Copy link
Contributor Author

doudou commented May 15, 2020

Hi @g-arjones. Thanks for putting the finger where it hurts.

The whole vera thing has not delivered for me. It's very hard to extend and is unsupported by essentially anyone. For instance, it will complain about missing spaces after [ in a string because it believes it is an array.

I've started toying with using clang-format again. Someone had an interesting idea, that is to run clang-format on a copy of the file and let people look at the diff. I think there's something there, and that would definitely be a better way forward. I've come with a minimal google-based configuration that is close to existing code: https://gist.github.com/doudou/a240627b5921c1ca6321391629fae273

Why am I talking about it now ? I just realized I haven't fixed the .cpp files as well, which makes no sense (if we're fixing the templates, let's do it completely). I'd like to us to agree to a way with formatting (and a configuration) and then move forward with this change, this time making sure all the generated C++ code passes whatever formatting solution we picked.

If you agree, let's put this as draft, and move the discussion to the team discussion ?

@g-arjones
Copy link
Contributor

Yeah, well, I have spent A LOT of time looking into all the options multiple times now and I really believe vera++ is the best one (cpplint is a close second option but it's not extendable at all) if we want "live" linting (and I agree it sucks, btw). I don't think turning clang-format into a linter is feasible if you want meaningful error/warning messages. The reality for me is that linting C/C++ is very hard and no one cares enough to try to fix the situation.

Now, what I believe would provide the best value (with a tradeoff in usability) is using clang-tidy and getting rid of vera++. It's highly customizable and has plenty of checks but requires the tool to be able to "build" the code. Since it supports compilation databases, the vscode extension could wait until it finds one to actually try to lint the code (that's the drawback in usability I mentioned before). It's supported by cmake with a flag so we could consider integrating with the CI through rock macros as well.

If you agree, let's put this as draft, and move the discussion to the team discussion ?

Fine 👍

@g-arjones
Copy link
Contributor

it supports compilation databases

Headers usually don't show up in those so there is that.. :(

@doudou doudou marked this pull request as draft May 15, 2020 18:43
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.

2 participants