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

Improve naming consistency #456

Merged
merged 12 commits into from
Nov 26, 2019
Merged

Improve naming consistency #456

merged 12 commits into from
Nov 26, 2019

Conversation

banesullivan
Copy link
Member

@banesullivan banesullivan commented Nov 20, 2019

Resolve #429 to improve naming consistency for *_scalars and *_array. This should be good to go and has deprecation warnings for the old methods to keep backward compatibility.

@pyvista/developers, anyone have time to double-check all of this? This needs to land before the 0.23.0 release.

@banesullivan banesullivan added the proposed-change Something with regards to the API or internal structure is changing. label Nov 20, 2019
@banesullivan banesullivan self-assigned this Nov 20, 2019
@banesullivan banesullivan added the review-critical For changes that might have serious implications - always good to get a second set of careful eyes. label Nov 20, 2019
@banesullivan banesullivan changed the title Improve naming consistency 🚧 Improve naming consistency Nov 20, 2019
@banesullivan banesullivan added this to the 0.23.0 milestone Nov 20, 2019
@banesullivan banesullivan changed the title 🚧 Improve naming consistency Improve naming consistency Nov 22, 2019
Copy link
Contributor

@imsodin imsodin left a comment

Choose a reason for hiding this comment

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

In many docstrings and comments it's still scalar, not scalars, that's used. That's definitely minor, but somewhat inconsistent. Maybe worth running sed -e 's/ scalar / scalars /' over it.

pyvista/core/common.py Outdated Show resolved Hide resolved
pyvista/core/common.py Outdated Show resolved Hide resolved
pyvista/utilities/helpers.py Outdated Show resolved Hide resolved
pyvista/utilities/helpers.py Outdated Show resolved Hide resolved
banesullivan and others added 4 commits November 25, 2019 12:12
Co-Authored-By: Simon Frei <freisim93@gmail.com>
Co-Authored-By: Simon Frei <freisim93@gmail.com>
@banesullivan
Copy link
Member Author

@imsodin, thanks for the review! And thanks for catching my sloppy copy/paste errors - that's what happens when I work on this stuff late at night

@banesullivan
Copy link
Member Author

In many docstrings and comments it's still scalar, not scalars, that's used.

@imsodin, I just committed a change that updates the docstrings use of scalar to either scalars or array depending on the context

Copy link
Contributor

@imsodin imsodin left a comment

Choose a reason for hiding this comment

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

@imsodin, thanks for the review! And thanks for catching my sloppy copy/paste errors - that's what happens when I work on this stuff late at night

I hope you don't take my remarks as criticism - I would be surprised if there wasn't any ;)

I am approving this, but I only skimmed (briefly) through it. However I think any remaining issues should be cosmetic and more reviewer time is better spent on functional stuff.

@banesullivan
Copy link
Member Author

I hope you don't take my remarks as criticism - I would be surprised if there wasn't any ;)

Oh, I definitely don't - your feedback is much appreciated! I am awful at catching my own mistakes these days.

I am approving this, but I only skimmed (briefly) through it. However I think any remaining issues should be cosmetic and more reviewer time is better spent on functional stuff.

Fair - I think all is good at this point. @GuillaumeFavelier, it would be great to have your eyes on this and perhaps use this PR as a place to implement to the decorator in #469

@banesullivan banesullivan merged commit 5851fae into master Nov 26, 2019
@banesullivan banesullivan deleted the naming branch December 11, 2019 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposed-change Something with regards to the API or internal structure is changing. review-critical For changes that might have serious implications - always good to get a second set of careful eyes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Find and resolve all naming inconsistencies
2 participants