Skip to content

Fix pre-commit#215

Merged
atztogo merged 4 commits intospglib:developfrom
LecrisUT:fix/213
Jan 14, 2023
Merged

Fix pre-commit#215
atztogo merged 4 commits intospglib:developfrom
LecrisUT:fix/213

Conversation

@LecrisUT
Copy link
Copy Markdown
Collaborator

@LecrisUT LecrisUT commented Jan 12, 2023 •
edited
Loading

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
Copy Markdown
Collaborator Author

LecrisUT commented Jan 12, 2023

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

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jan 12, 2023 •
edited
Loading

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
lan496
lan496 reviewed Jan 13, 2023
View reviewed changes
@atztogo
Copy link
Copy Markdown
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?

@LecrisUT
Copy link
Copy Markdown
Collaborator Author

LecrisUT commented Jan 13, 2023

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

@LecrisUT LecrisUT marked this pull request as ready for review January 14, 2023 13:15
lan496
lan496 approved these changes Jan 14, 2023
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

atztogo commented Jan 14, 2023

Merged. Thanks @LecrisUT and @lan496.

@LecrisUT
Copy link
Copy Markdown
Collaborator Author

LecrisUT commented Jan 14, 2023

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

Reviewers

@lan496 lan496 lan496 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2.1

Development

Successfully merging this pull request may close these issues.

pre-commit error report after PR #210

4 participants

@LecrisUT @codecov-commenter @atztogo @lan496