Skip to content

GH-34037: [Python][Docs] Fix Table.drop docstring #34038

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
Merged
jorisvandenbossche merged 2 commits into from
Feb 7, 2023
Merged

GH-34037: [Python][Docs] Fix Table.drop docstring #34038

jorisvandenbossche merged 2 commits into from
Feb 7, 2023

Conversation

@Fokko
Copy link
Contributor

@Fokko Fokko commented Feb 3, 2023 •
edited
Loading

It throws errors in the CI, while I was working on other docstrings:

pyarrow.lib.Table.drop
-> pyarrow.lib.Table.drop(self, columns)
PR01: Parameters {'columns'} not documented

pyarrow.lib.Table.drop
-> pyarrow.lib.Table.drop(self, columns)
PR01: Parameters {'columns'} not documented

pyarrow.lib.Table.drop
-> pyarrow.lib.Table.drop(self, columns)
PR01: Parameters {'columns'} not documented

Total number of docstring violations: 3

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@Fokko Fokko requested a review from AlenkaF as a code owner February 3, 2023 20:44
@github-actions
Copy link

github-actions bot commented Feb 3, 2023

@github-actions
Copy link

github-actions bot commented Feb 3, 2023

⚠️ GitHub issue #34037 has been automatically assigned in GitHub to PR creator.

@Fokko Fokko force-pushed the fd-fix-drop-docstring branch 2 times, most recently from 3952651 to 8251f42 Compare February 3, 2023 21:04
@Fokko
Copy link
Contributor Author

Fokko commented Feb 3, 2023 •
edited
Loading

cc @jorisvandenbossche @westonpace this PR fixes the CI on master 👍🏻

This was referenced Feb 5, 2023
Merged
Merged
@kou
Copy link
Member

kou commented Feb 6, 2023

@github-actions crossbow submit test-ubuntu-default-docs

@github-actions
Copy link

github-actions bot commented Feb 6, 2023

Revision: 8251f42b49d15bcaa04257bc3286c23e3bfa731b

Submitted crossbow builds: ursacomputing/crossbow @ actions-639508a0f9

Task Status
test-ubuntu-default-docs Azure

@westonpace
Copy link
Member

westonpace commented Feb 6, 2023

The test-ubuntu-default-docs job was missing substrait but that was fixed two days ago. Maybe rebase this?

As for the fix, was the intention to deprecate pyarrow.Table.drop? It isn't clear to me from the original ticket though it would make sense. CC @danepitkin @amol-

@Fokko
apacheGH-34037: [Python][Docs] Fix Table.drop docstring
bd63def 
It throws errors in the CI:

```
pyarrow.lib.Table.drop
-> pyarrow.lib.Table.drop(self, columns)
PR01: Parameters {'columns'} not documented

pyarrow.lib.Table.drop
-> pyarrow.lib.Table.drop(self, columns)
PR01: Parameters {'columns'} not documented

pyarrow.lib.Table.drop
-> pyarrow.lib.Table.drop(self, columns)
PR01: Parameters {'columns'} not documented

Total number of docstring violations: 3
```
@Fokko Fokko force-pushed the fd-fix-drop-docstring branch from 8251f42 to bd63def Compare February 6, 2023 15:53
@Fokko
Copy link
Contributor Author

Fokko commented Feb 6, 2023

@westonpace Thanks, I've just rebased the PR.

I got the impression that it was to be deprecated according to the original docstring. My personal opinion is that having duplicate methods will confuse users, and there should be a single way of dropping columns. Calling .drop on a table implies to me that it will drop the whole table. Therefore I figured it made sense to deprecate it for now.

@westonpace
Copy link
Member

westonpace commented Feb 6, 2023

@github-actions crossbow submit test-ubuntu-default-docs

@github-actions
Copy link

github-actions bot commented Feb 6, 2023

Revision: bd63def

Submitted crossbow builds: ursacomputing/crossbow @ actions-b7a4d96192

Task Status
test-ubuntu-default-docs Azure

@mapleFU
Copy link
Member

mapleFU commented Feb 7, 2023

(So, can this patch be merged? Seems some of my patch is blocked by pydoc...)

@coryan coryan mentioned this pull request Feb 7, 2023
Merged
@kou kou mentioned this pull request Feb 7, 2023
Closed
@wgtmac
Copy link
Member

wgtmac commented Feb 7, 2023

(So, can this patch be merged? Seems some of my patch is blocked by pydoc...)

Same here: https://github.com/apache/arrow/actions/runs/4110851940/jobs/7094056424

@Fokko Fokko mentioned this pull request Feb 7, 2023
Closed
jorisvandenbossche
jorisvandenbossche approved these changes Feb 7, 2023
View reviewed changes
Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

@Fokko Thanks for the PR!

I am personally fine with also actually deprecating the method (instead of keeping it as an alias), but I removed that here from this PR to just focus on fixing the docstring validation so that this can be merged quickly (on the deprecation I had some minor comments).

@jorisvandenbossche jorisvandenbossche added this to the 12.0.0 milestone Feb 7, 2023
@jorisvandenbossche jorisvandenbossche merged commit 1ab1f6f into apache:master Feb 7, 2023
@Fokko Fokko deleted the fd-fix-drop-docstring branch February 7, 2023 08:35
@Fokko
Copy link
Contributor Author

Fokko commented Feb 7, 2023 •
edited
Loading

I am personally fine with also actually deprecating the method (instead of keeping it as an alias), but I removed that here from this PR to just focus on fixing the docstring validation so that this can be merged quickly (on the deprecation I had some minor comments).

No problem at all, feel free to change such a PR any time to get it in. Getting the CI green is the most important part 🚀 Thanks for merging!

@ursabot
Copy link

ursabot commented Feb 7, 2023

Benchmark runs are scheduled for baseline = d1e5cb5 and contender = 1ab1f6f. 1ab1f6f is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.28% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.22% ⬆️0.03%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 1ab1f6f1 ec2-t3-xlarge-us-east-2
[Finished] 1ab1f6f1 test-mac-arm
[Finished] 1ab1f6f1 ursa-i9-9960x
[Finished] 1ab1f6f1 ursa-thinkcentre-m75q
[Finished] d1e5cb55 ec2-t3-xlarge-us-east-2
[Failed] d1e5cb55 test-mac-arm
[Finished] d1e5cb55 ursa-i9-9960x
[Finished] d1e5cb55 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jorisvandenbossche jorisvandenbossche jorisvandenbossche approved these changes

@AlenkaF AlenkaF Awaiting requested review from AlenkaF

Assignees

No one assigned

Labels

Component: Python

Projects

None yet

Milestone

12.0.0

Development

Successfully merging this pull request may close these issues.

[Python][Docs] Fix docstring violations for Table.drop

7 participants

@Fokko @kou @westonpace @mapleFU @wgtmac @ursabot @jorisvandenbossche