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

renamed decorator as per issue #233 #235

Merged
merged 2 commits into from
Jun 26, 2020

Conversation

Aditya1001001
Copy link
Contributor

-[x] Rename set_check_statistics to register_check_statistics as per #233

@cosmicBboy
Copy link
Collaborator

cosmicBboy commented Jun 26, 2020

Thanks @Aditya1001001! I think you missed one on line 446

@cosmicBboy
Copy link
Collaborator

(FYI you can catch these kinds of things locally by running tests)

@Aditya1001001
Copy link
Contributor Author

@cosmicBboy I made the change and ran the decorator test script successfully. However, I did encounter a problem while simply running all tests:

ImportError while importing test module 'Z:\issue\pandera\tests\test_docs_setting_column_widths.py'
Hint: make sure your test modules/packages have valid Python names.
Traceback:
tests\test_docs_setting_column_widths.py:7: in <module>
    from docs.source import conf
ModuleNotFoundError: No module named 'docs.source'

Can you help with this? It will make testing my future PRs easier.

@cosmicBboy
Copy link
Collaborator

cosmicBboy commented Jun 26, 2020

interesting. are you running pytest tests from the root of the repo?

A full system report (OS, python version, etc) would be useful too

@codecov-commenter
Copy link

codecov-commenter commented Jun 26, 2020

Codecov Report

Merging #235 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #235   +/-   ##
=======================================
  Coverage   96.33%   96.33%           
=======================================
  Files          15       15           
  Lines        1284     1284           
=======================================
  Hits         1237     1237           
  Misses         47       47           
Impacted Files Coverage Δ
pandera/checks.py 98.41% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5036676...2fda593. Read the comment docs.

@Aditya1001001
Copy link
Contributor Author

Aditya1001001 commented Jun 26, 2020

@cosmicBboy Yes I did run the tests from the root of the reop both times, it gave the aforementioned error while running all tests and worked when I just ran "pytest tests/test_decorators.py". OS is Windows 10 Pro, and I'm using Python 3.7.4 in a conda environment.

@cosmicBboy
Copy link
Collaborator

The error you're seeing is because somehow the docs/source/conf.py isn't being recognized as a module in your environment.

Can you do from docs.source import conf in a python REPL successfully?

@Aditya1001001
Copy link
Contributor Author

I tried to run it outside of my conda environment and that worked, why would that be? And how can I deal with that issue?

@cosmicBboy
Copy link
Collaborator

outside of my conda environment

did it work in the base conda environment, or do you mean the system python env?

@Aditya1001001
Copy link
Contributor Author

Aditya1001001 commented Jun 26, 2020

Yes, I meant the system python env, it did not work in the base conda env.

@cosmicBboy
Copy link
Collaborator

@Aditya1001001 would you mind creating a new issue for this? I'm unable to reproduce the failing test that you're seeing, and the Windows CI tests are passing, so I'm not sure what's going on. This PR looks good tho, I'll merge this in and discuss this on the new issue.

@cosmicBboy
Copy link
Collaborator

Also, thanks for the PR, and congrats on your first contribution to pandera! 🚀

@cosmicBboy cosmicBboy merged commit 8efc536 into unionai-oss:master Jun 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants