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 pre-commit #215

Merged
merged 4 commits into from Jan 14, 2023
Merged

Fix pre-commit #215

merged 4 commits into from Jan 14, 2023

Conversation

LecrisUT
Copy link
Collaborator

@LecrisUT LecrisUT commented Jan 12, 2023

Closes #213

Note that there are some pre-commit that have not been fixed.

(Currently depends on #214 because I didn't rebase that out)

@LecrisUT
Copy link
Collaborator Author

@atztogo Can you help or assign someone to help with some of the C errors like malloc etc.?

@codecov-commenter
Copy link

codecov-commenter commented Jan 12, 2023

Codecov Report

Base: 64.00% // Head: 64.00% // No change to project coverage 👍

Coverage data is based on head (14c3b57) compared to base (fb55b8f).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #215   +/-   ##
========================================
  Coverage    64.00%   64.00%           
========================================
  Files           18       18           
  Lines         1328     1328           
========================================
  Hits           850      850           
  Misses         478      478           
Impacted Files Coverage Δ
setup.py 0.00% <0.00%> (ø)
spglib/spglib.py 0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@LecrisUT LecrisUT marked this pull request as draft January 12, 2023 18:25
@atztogo
Copy link
Collaborator

atztogo commented Jan 13, 2023

to help with some of the C errors like malloc etc.?

This part, I don't understand. What do you want? Have you met malloc errors?

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
@LecrisUT
Copy link
Collaborator Author

This part, I don't understand. What do you want? Have you met malloc errors?

If we run pre-commit now, clang-tidy fails due to various errors, one of them was a malloc issue, though I can't find it right now

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
@LecrisUT LecrisUT marked this pull request as ready for review January 14, 2023 13:15
Copy link
Member

@lan496 lan496 left a comment

Choose a reason for hiding this comment

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

Thank you so much for the quick fix! I confirm many warnings occur by running clang-tidy for all files, but it will be better to be addressed them in another PR.

Note for me: cmake-pre-commit-hooks creates a build directory (currently _build-pre-commit specified in .pre-commit-config.yaml) and compile_commands.json.

@atztogo I also think this PR is ready to be merged.

@atztogo atztogo merged commit 1d3e5dc into spglib:develop Jan 14, 2023
@atztogo
Copy link
Collaborator

atztogo commented Jan 14, 2023

Merged. Thanks @LecrisUT and @lan496.

@LecrisUT
Copy link
Collaborator Author

Note for me: cmake-pre-commit-hooks creates a build directory (currently _build-pre-commit specified in .pre-commit-config.yaml) and compile_commands.json.

Yeah, it can also use the pre-configured build directories. I chose against it because it might not collect all source code, e.g. if it's build with WITH_FORTRAN=OFF.

@LecrisUT LecrisUT deleted the fix/213 branch January 20, 2023 09:36
@LecrisUT LecrisUT added this to the 2.1 milestone Feb 21, 2023
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.

pre-commit error report after PR #210
4 participants