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

FEAT: Add docstring check to CI for an initial subset files #1996

Merged
merged 3 commits into from
Oct 29, 2019
Merged

FEAT: Add docstring check to CI for an initial subset files #1996

merged 3 commits into from
Oct 29, 2019

Conversation

xmnlab
Copy link
Contributor

@xmnlab xmnlab commented Oct 12, 2019 •
edited

In this PR:

  • Fixed omniscidb docstrings
  • Fixed some docstrings
  • Added pydocstyle check for ./ibis/.py and ./ibis/omniscidb/*.py

Current issues with pydocstyle:

@xmnlab xmnlab changed the title Add docstring check to CI and git pre-commit hook FEAT: Add docstring check to CI for an initial subset files Oct 22, 2019
@xmnlab xmnlab marked this pull request as ready for review October 22, 2019 04:47
@xmnlab xmnlab self-assigned this Oct 22, 2019
@xmnlab
Copy link
Contributor Author

xmnlab commented Oct 23, 2019 •
edited

I will try each week to fix docstring for some backend (addressed in new PRs).

@xmnlab xmnlab mentioned this pull request Oct 25, 2019
Closed
@xmnlab xmnlab mentioned this pull request Oct 29, 2019
Closed
@toryhaavik
Copy link
Contributor

The tests are failing with /home/vsts/work/_temp/b85e2339-eab9-4da6-8e4c-3f49be4fa0ab.sh: line 1: syntax error near unexpected token `(' /home/vsts/work/_temp/b85e2339-eab9-4da6-8e4c-3f49be4fa0ab.sh: line 1: `docker-compose run ibis pydocstyle --match-dir=(ibis|omniscidb)'

@xmnlab
Copy link
Contributor Author

xmnlab commented Oct 29, 2019

thanks @toryhaavik I am working on that. it is missing the quotes. thanks!

@xmnlab
Copy link
Contributor Author

xmnlab commented Oct 29, 2019

all the tests passed now :)

toryhaavik
toryhaavik approved these changes Oct 29, 2019
View reviewed changes
Copy link
Contributor

@toryhaavik toryhaavik left a comment

Choose a reason for hiding this comment

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

Thanks @xmnlab this is great work!

ibis/omniscidb/tests/conftest.py
@@ -17,6 +20,12 @@

@pytest.fixture(scope='module')
def con():
"""Define a connection fixture.
Copy link
Contributor

Choose a reason for hiding this comment

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

we may want to consider loosening the restriction for test code. you shouldn't necessarily need to write a docstring for a test method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIRC the default for match parameter exclude tests for the checking. I just add that to help people to understand easily (as much as possible) the tests. not sure if it is the right direction ...

also there is an issue related to the documentation and tests experiences: #1989

@toryhaavik toryhaavik merged commit 4a1f5bc into ibis-project:master Oct 29, 2019
@xmnlab
Copy link
Contributor Author

xmnlab commented Oct 29, 2019

thanks for the review and the merge @toryhaavik !

@xmnlab xmnlab deleted the add-docstring-check branch October 29, 2019 20:05
@xmnlab xmnlab added ci Continuous Integration issues or PRs docs Documentation related issues or PRs omnisci labels Nov 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@toryhaavik toryhaavik toryhaavik approved these changes

@jreback jreback Awaiting requested review from jreback

@scottcode scottcode Awaiting requested review from scottcode

Assignees

@xmnlab xmnlab

Labels
ci Continuous Integration issues or PRs docs Documentation related issues or PRs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@xmnlab @toryhaavik