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.

Sign up for GitHub

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

Jump to bottom

Fix the docstring of the charts.py #3682

Merged
merged 30 commits into from
Jan 18, 2023
Merged

Fix the docstring of the charts.py #3682

merged 30 commits into from
Jan 18, 2023

Conversation

tkoyama010
Copy link
Member

Overview

Fix the docstring of the charts.py

Details

  • None

@github-actions github-actions bot added the maintenance Low-impact maintenance activity label Dec 5, 2022
@tkoyama010 tkoyama010 marked this pull request as ready for review December 5, 2022 13:54
@codecov
Copy link

codecov bot commented Dec 5, 2022 •
edited

Codecov Report

Merging #3682 (dead474) into main (9043cd1) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #3682   +/-   ##
=======================================
  Coverage   94.11%   94.11%           
=======================================
  Files          87       87           
  Lines       19108    19108           
=======================================
  Hits        17984    17984           
  Misses       1124     1124

dcbr
dcbr requested changes Dec 5, 2022
View reviewed changes
Copy link
Contributor

@dcbr dcbr left a comment

Choose a reason for hiding this comment

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

Thanks for this! Just some minor remarks on certain behaviour when the default is used.

pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Show resolved Hide resolved
pyvista/plotting/charts.py Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
tkoyama010 and others added 2 commits December 6, 2022 06:28
@tkoyama010 @dcbr
Apply suggestions from code review
ea99140 
Co-authored-by: dcbr <15089458+dcbr@users.noreply.github.com>
@tkoyama010
Fix error of black
a405819
tkoyama010
tkoyama010 commented Dec 6, 2022
View reviewed changes
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
tkoyama010
tkoyama010 commented Dec 6, 2022
View reviewed changes
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
dcbr
dcbr requested changes Dec 6, 2022
View reviewed changes
Copy link
Contributor

@dcbr dcbr left a comment

Choose a reason for hiding this comment

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

Suggestions for the failing doc build.

pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
@tkoyama010 @dcbr
Apply suggestions from code review
e6da2bb 
Co-authored-by: dcbr <15089458+dcbr@users.noreply.github.com>
@dcbr
Copy link
Contributor

dcbr commented Dec 6, 2022

Ok, the docs are built fine now, but the linking seems to be broken. Not sure how we can fix this :/

image

Also the markup for "np.zeros_like(x)" seems odd to me (and maybe some linking here would be helpful as well?)

image

@tkoyama010 tkoyama010 closed this Dec 7, 2022
@tkoyama010 tkoyama010 reopened this Dec 7, 2022
MatthewFlamm
MatthewFlamm reviewed Dec 10, 2022
View reviewed changes
Copy link
Contributor

@MatthewFlamm MatthewFlamm left a comment

Choose a reason for hiding this comment

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

I left two comments but they apply to a large number of changes

pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Show resolved Hide resolved
tkoyama010
tkoyama010 commented Dec 11, 2022
View reviewed changes
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
dcbr
dcbr reviewed Dec 11, 2022
View reviewed changes
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
@banesullivan @dcbr
Update pyvista/plotting/charts.py
46201e7 
Co-authored-by: dcbr <15089458+dcbr@users.noreply.github.com>
banesullivan
banesullivan reviewed Dec 16, 2022
View reviewed changes
Copy link
Member

@banesullivan banesullivan left a comment

Choose a reason for hiding this comment

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

empty strings and empty lists should be marked optional without a default value listed. My suggestions correct these (which is what they were originally)

pyvista/plotting/charts.py Show resolved Hide resolved
pyvista/plotting/charts.py Show resolved Hide resolved
pyvista/plotting/charts.py Show resolved Hide resolved
pyvista/plotting/charts.py Show resolved Hide resolved
pyvista/plotting/charts.py Show resolved Hide resolved
pyvista/plotting/charts.py Show resolved Hide resolved
pyvista/plotting/charts.py Show resolved Hide resolved
pyvista/plotting/charts.py Show resolved Hide resolved
pyvista/plotting/charts.py Show resolved Hide resolved
pyvista/plotting/charts.py Show resolved Hide resolved
@banesullivan banesullivan marked this pull request as draft December 16, 2022 22:40
tkoyama010 and others added 2 commits December 20, 2022 15:28
@tkoyama010 @banesullivan
Apply suggestions from code review
e790ab1 
Co-authored-by: Bane Sullivan <banesullivan@gmail.com>
@dcbr
Apply suggestions of code review
d152452
tkoyama010
tkoyama010 commented Jan 13, 2023
View reviewed changes
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
@tkoyama010 tkoyama010 marked this pull request as ready for review January 13, 2023 14:16
@tkoyama010 @banesullivan
Update pyvista/plotting/charts.py
6f1792d 
Co-authored-by: Bane Sullivan <banesullivan@gmail.com>
tkoyama010
tkoyama010 commented Jan 14, 2023
View reviewed changes
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
dcbr
dcbr reviewed Jan 14, 2023
View reviewed changes
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
@tkoyama010 @dcbr
Apply suggestions from code review
28d1ef1 
Co-authored-by: dcbr <15089458+dcbr@users.noreply.github.com>
dcbr
dcbr reviewed Jan 17, 2023
View reviewed changes
Copy link
Contributor

@dcbr dcbr left a comment

Choose a reason for hiding this comment

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

Last few changes, sorry didn't catch these last time.

pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
pyvista/plotting/charts.py Outdated Show resolved Hide resolved
dcbr
dcbr approved these changes Jan 17, 2023
View reviewed changes
Copy link
Contributor

@dcbr dcbr 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 for going through this!

akaszynski
akaszynski approved these changes Jan 18, 2023
View reviewed changes
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.

Thanks @tkoyama010!

@banesullivan banesullivan merged commit a7f3fbe into main Jan 18, 2023
@banesullivan banesullivan deleted the maint/fix-docstring-of-charts branch January 18, 2023 16:23
@banesullivan banesullivan mentioned this pull request Feb 1, 2023
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akaszynski akaszynski akaszynski approved these changes

@dcbr dcbr dcbr approved these changes

@banesullivan banesullivan banesullivan approved these changes

@MatthewFlamm MatthewFlamm Awaiting requested review from MatthewFlamm

Assignees
No one assigned
Labels
maintenance Low-impact maintenance activity
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@tkoyama010 @dcbr @banesullivan @akaszynski @MatthewFlamm