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

Convert to modern include guard #882 #891

Merged
merged 16 commits into from
Dec 22, 2021

Conversation

predystopic-dev
Copy link
Contributor

@predystopic-dev predystopic-dev commented Dec 4, 2021

Description

Please explain the changes you made, including a reference to the related issue if applicable

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

It is my first contribution, I just thought this issue seems trivial enough to do, but I might have made some really dumb mistakes or messed up something completely, please let me know if I did any of that, and help me fix my mistakes please. From what I understood, I was just supposed to look through all header files and replace include guards with #pragma once but I am not sure if it was actually what I thought, sorry if what i did was dumb/wrong/bad, but please let me know the correct way of doing stuff so I don't mess up in future.

PS- I don't know how to fill the checklist thing, not that it seems relevant for this issue but what am I supposed to put in the brackets?

Comment on lines 37 to 38
//#ifndef XML_TESTDATA_LOADER_H
//#define XML_TESTDATA_LOADER_H
Copy link
Member

Choose a reason for hiding this comment

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

Please remove all these lines instead of just commenting them out.

@tylerjw
Copy link
Member

tylerjw commented Dec 6, 2021

I resolved the merge conflict but that created a clang-format issue. If you'd like I can fix that issue for you or you can do it by running pre-commit locally. Pre-commit and clang-fromat can be installed like this:

sudo apt install clang-format-10
pip3 install -U pre-commit

Then to run it locally before you commit (this will make so all the formatting passes the pre-commit check). You do this in your local checkout of moveit2:

pre-commit run -a

@codecov
Copy link

codecov bot commented Dec 6, 2021

Codecov Report

Merging #891 (7ba669b) into main (b41a741) will increase coverage by 0.02%.
The diff coverage is n/a.

❗ Current head 7ba669b differs from pull request most recent head fa87c38. Consider uploading reports for the commit fa87c38 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #891      +/-   ##
==========================================
+ Coverage   57.91%   57.92%   +0.02%     
==========================================
  Files         310      310              
  Lines       26069    26069              
==========================================
+ Hits        15095    15098       +3     
+ Misses      10974    10971       -3     
Impacted Files Coverage Δ
...industrial_motion_planner/joint_limits_extension.h 100.00% <ø> (ø)
..._motion_planner/joint_limits_interface_extension.h 76.93% <ø> (ø)
...pilz_industrial_motion_planner_testutils/basecmd.h 100.00% <ø> (ø)
..._motion_planner_testutils/cartesianconfiguration.h 43.75% <ø> (ø)
...lanner_testutils/cartesianpathconstraintsbuilder.h 0.00% <ø> (ø)
.../pilz_industrial_motion_planner_testutils/center.h 100.00% <ø> (ø)
...de/pilz_industrial_motion_planner_testutils/circ.h 41.67% <ø> (ø)
...ndustrial_motion_planner_testutils/circauxiliary.h 50.00% <ø> (ø)
...ustrial_motion_planner_testutils/exception_types.h 0.00% <ø> (ø)
..._planner_testutils/goalconstraintsmsgconvertible.h 100.00% <ø> (ø)
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b41a741...fa87c38. Read the comment docs.

@predystopic-dev
Copy link
Contributor Author

I resolved the merge conflict but that created a clang-format issue. If you'd like I can fix that issue for you or you can do it by running pre-commit locally. Pre-commit and clang-fromat can be installed like this:

sudo apt install clang-format-10
pip3 install -U pre-commit

Then to run it locally before you commit (this will make so all the formatting passes the pre-commit check). You do this in your local checkout of moveit2:

pre-commit run -a

I am getting dpkg and apt errors on my machine while installing clang-format-10, probably some update issues, I can try fixing it later after fixing these errors or maybe do it yourself, I think I am just supposed to run pre-commit run -a and fix whatever is returned as "failed"?

@abisiva-pn
Copy link

Running pre-commit should format all the files for you. So when it says "failed", it introduces the fixes for it too. Try running pre-commit again and you will see it pass.

@tylerjw
Copy link
Member

tylerjw commented Dec 8, 2021

This is failing pre-commit again. Would you like me to fix it or would you like to try?

There are instructions here for how to install and use it but the gist of it is what I told you earlier. You need to install clang-format-10 with pre-commit. Generally, I install clang-format-10 with apt and pre-commit with pip3 with the -U flag so it is installed into my home directory.

@predystopic-dev
Copy link
Contributor Author

just saw that, it showed passed when I ran it before pushing it. I tried again and it still shows passed on my machine.

@predystopic-dev
Copy link
Contributor Author

I ran pre-commit again same as before but it still seems that it isn't working? I don't understand where the problem is.

@JafarAbdi
Copy link
Member

I ran pre-commit again same as before but it still seems that it isn't working? I don't understand where the problem is.

I just pushed a commit that fixes the formating

@tylerjw
Copy link
Member

tylerjw commented Dec 8, 2021

Previously you stated:

I am getting dpkg and apt errors on my machine while installing clang-format-10

I bet you still don't have clang-format-10 installed which is what you need to fix the formatting.

@tylerjw tylerjw merged commit bf82931 into moveit:main Dec 22, 2021
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.

4 participants