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

essential ci fixes and pylint changes #138

Merged
merged 14 commits into from
Jan 8, 2020

Conversation

mastersplinter
Copy link
Collaborator

@mastersplinter mastersplinter commented Jan 8, 2020

The Python 3.5 CI process has never worked correctly - the previous mixture of Conda and Pip dependencies caused the Python version to be upgrade to Python 3.7.5 and this can only be seen in the Raw job logs on Travis. This means the current pandera versions are not tested properly for Python 3.5 support - this PR fixes that.

Fixing the CI is complicated because Pylint is currently 2.4.4 but the last version that supports 3.5 is 2.1.1 and that has different linting rules which cause 3.5-specific failures.

This PR:

  • improves the breadth of CI test coverage and closes Improve breadth of CI tests to cover specific dependency versions instead of latest #131:
    • creates specific conda environment yml files in ci/deps
    • simplifies the Travis Pipeline to only use environment.yml files closes Switch to environment.yml or remove conda? #130
    • deprecates requirements.txt
    • borrows a script from pandas to auto-create a requirements-dev.txt which is generated from the base environment.yml file and not manually maintained.
    • removes most specific version-number requirements, because deprecating Python 2 has resulted in these no longer being required
  • fixes pylint errors across 3.5 and later python versions:
    • Either resolves R1721 unnecessary-comprehension, W0613 unused-argument, R0201 no-self-use, R0205 useless-object-inheritance, R0801 duplicate-code
    • or makes these exceptions function-specific with explanations for why it makes sense to keep them.
  • Re-enables the CI tests that check for the fixed pylint errors

This leaves the following pylint errors where @cosmicBboy I need your input.

  • R0913 Too many arguments (%s/%s) Used when a function or method takes too many arguments.
  • W0222 signature-differs Signature differs from %s %r method Used when a method signature is different than in the implemented interface or in an overridden method.

There are similar options for each, but with different consequences:

Fixing R0913 too-many-arguments:

  • Option 1: Leave it as is; disabled in messages control of .pylintrc
    • may cause problems in future
  • Option 2: Selectively disable the errors on existing public functions, remove it from messages control
    • avoids problems in future, leaves the API non pylint compliant which seems messy
  • Option 3: Change the public API of pandera; limiting public functions to only 5 inputs
    • would incur breaking changes, but not necessarily a bad thing if the removed arguments were minor

Fixing W0222 signature-differs:

  • Option 1: Leave it as is; disabled in messages control of .pylintrc
    • may cause problems in future
  • Option 2: Selectively disable the errors
    • avoids problems in future, leaves the API non pylint compliant which seems messy
  • Option 3: Change the way inheritance works from SeriesSchemaBase
    • the inconsistent inheritance is challenging for new developers - I found it confusing when originally looking at pandera, so changing the inheritance approach could make sense.

Can I also suggest you squash merge this PR with all of the linting fixes into one large commit to ensure a clean commit history:
image

@mastersplinter mastersplinter force-pushed the feature/enhanced_CI_with_pylint_fixes branch from 28e6716 to 1445b4a Compare January 8, 2020 11:30
@codecov-io
Copy link

codecov-io commented Jan 8, 2020

Codecov Report

Merging #138 into master will increase coverage by 0.02%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #138      +/-   ##
==========================================
+ Coverage   95.92%   95.95%   +0.02%     
==========================================
  Files           8        8              
  Lines         442      445       +3     
==========================================
+ Hits          424      427       +3     
  Misses         18       18
Impacted Files Coverage Δ
pandera/schema_components.py 93.61% <0%> (ø) ⬆️
pandera/checks.py 95.74% <100%> (+0.04%) ⬆️
pandera/schemas.py 100% <100%> (ø) ⬆️
pandera/hypotheses.py 88.23% <0%> (+0.23%) ⬆️

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 b52e18d...1445b4a. Read the comment docs.

@mastersplinter
Copy link
Collaborator Author

as before - I think codecov failures are false.

@cosmicBboy
Copy link
Collaborator

cosmicBboy commented Jan 8, 2020

excellent!

Fixing R0913 too-many-arguments:
Option 1: Leave it as is; disabled in messages control of .pylintrc may cause problems in future
Option 2: Selectively disable the errors on existing public functions, remove it from messages control avoids problems in future, leaves the API non pylint compliant which seems messy
Option 3: Change the public API of pandera; limiting public functions to only 5 inputs
would incur breaking changes, but not necessarily a bad thing if the removed arguments were minor

I think 5 inputs for functions/methods are quite restrictive and we should stay with option 1, at least for the foreseeable future.

Fixing W0222 signature-differs:
Option 1: Leave it as is; disabled in messages control of .pylintrc may cause problems in future
Option 2: Selectively disable the errors avoids problems in future, leaves the API non pylint compliant which seems messy
Option 3: Change the way inheritance works from SeriesSchemaBase the inconsistent inheritance is challenging for new developers - I found it confusing when originally looking at pandera, so changing the inheritance approach could make sense.

I created a separate task to tackle this issue 139... I do think the __call__ method for schema and schema_component classes can be cleaned up and made more consistent, I think this can probably be resolved without making breaking changes to the public API.

@mastersplinter mastersplinter merged commit c726a26 into master Jan 8, 2020
@mastersplinter mastersplinter deleted the feature/enhanced_CI_with_pylint_fixes branch January 8, 2020 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants