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 generate_cell_offsets test #2708

Merged
merged 1 commit into from
May 27, 2022
Merged

Fix generate_cell_offsets test #2708

merged 1 commit into from
May 27, 2022

Conversation

StefRe
Copy link
Contributor

@StefRe StefRe commented May 27, 2022

Overview

Fix generate_cell_offsets test to make it check at least anything (shortcut method generate_cell_offsets returns the same offsets as the explicit generate_cell_offsets_loop method.

Resolves #2211.

@github-actions github-actions bot added the bug Uh-oh! Something isn't working as expected. label May 27, 2022
@codecov
Copy link

codecov bot commented May 27, 2022

Codecov Report

Merging #2708 (865b913) into main (bcf1501) will increase coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2708      +/-   ##
==========================================
+ Coverage   93.70%   93.76%   +0.05%     
==========================================
  Files          75       75              
  Lines       16178    16148      -30     
==========================================
- Hits        15160    15141      -19     
+ Misses       1018     1007      -11     

@akaszynski
Copy link
Member

Thanks for your contribution!

Please be sure to install pre-commit following our directions in the Contributing Style Guide. This way we can all automatically follow a similar documentation format.

After installing it, you'll want to run pre-commit run --all-files to clean up this PR. Should take a second and then I can approve it.

to make it check at least anything (shortcut method generate_cell_offsets
returns the same offsets as the explicit generate_cell_offsets_loop method.
@StefRe
Copy link
Contributor Author

StefRe commented May 27, 2022

@akaszynski Sorry and thanks for the explanation (black added a trailing comma in the function call which I personally find a bit unusual, but OK).

I had an error installing pre-commit which could be easily resolved using the workaround described in pre-commit/pre-commit#2178 (comment). Maybe it's worth - for the time being - to add a note with this link to the documentation in https://github.com/pyvista/pyvista/blob/main/CONTRIBUTING.rst#style-checking.

Copy link
Member

@akaszynski akaszynski left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@akaszynski akaszynski enabled auto-merge (squash) May 27, 2022 19:17
@akaszynski akaszynski merged commit 5bd65aa into pyvista:main May 27, 2022
@StefRe StefRe deleted the patch-1 branch May 27, 2022 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Uh-oh! Something isn't working as expected.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix generate_cell_offsets test
2 participants