Skip to content

Travis CI: Add Python 3.7 and flake8 tests - #20

Closed
cclauss wants to merge 13 commits into
garethdmm:masterfrom
cclauss:patch-1
Closed

Travis CI: Add Python 3.7 and flake8 tests#20
cclauss wants to merge 13 commits into
garethdmm:masterfrom
cclauss:patch-1

Conversation

@cclauss

@cclauss cclauss commented Jun 24, 2019

Copy link
Copy Markdown

We will run Python 3 in allow_failures mode until the tests are passing.

Flake8 will stop the build if there are Python syntax errors or undefined names.

Four undefined names (probably missing imports, typos, etc.) to be resolved on Python 2:

standard_deviation = np.std(relevant_values)
mean = np.mean(relevant_values)
standard_deviation = numpy.std(relevant_values)
mean = numpy.mean(relevant_values)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It's a convention among Python developers to use np for numpy. It will be understood by everybody :)

@cclauss cclauss Jun 24, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes but np is not imported or defined in this file -- there is no import numpy as np in this file. Instead, in this file there is only inport numpy and several existing calls to numpy.xyz()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

my bad, didn't notice the missing import :)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

flake8 is awesome for catching things like this that we humans so easily tend to miss.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This code is honestly deprecated, it could probably be removed entirely. That's likely why there was a syntax error in there.

@cclauss

cclauss commented Jun 24, 2019

Copy link
Copy Markdown
Author

@zeapo If you know how to fix and of the four remaining undefined names in Python 2, please open a PR to close them.

@zeapo

zeapo commented Jun 24, 2019

Copy link
Copy Markdown

@cclauss I'm not part of the project. I'm just like you looking forward to see it run on python3.
However, here are my speculations on the missing names (if any member of the project agree, I can open a PR to fix this):

./gryphon/lib/analysis/legacy/standard_deviation.py:9:23: F821 undefined name 'date'
        sddate.append(date[x])

This function is a copy/paste from another file : https://github.com/garethdmm/gryphon/blob/master/gryphon/lib/analysis/legacy/bollinger_bands.py where the date is a global variable.
Morevoer, the standard_deviation.py file is never used in the project. I think that it can be safely removed.

./gryphon/lib/analysis/legacy/bollinger_bands.py:32:18: F821 undefined name 'movingaverage'
        curSMA = movingaverage(closep[x-tff:x],tff)[-1]

This looks like this function: https://github.com/garethdmm/gryphon/blob/master/gryphon/lib/analysis/legacy/ease_of_movement.py#L12

./gryphon/dashboards/util/queries.py:98:12: F821 undefined name 'get_strategy_latest_order_for_period'
    return get_strategy_latest_order_for_period(
---
./gryphon/dashboards/util/queries.py:101:9: F821 undefined name 'start_time'
        start_time,

The function get_multi_latest_order that calls the missing functions is never used.

Comment thread .travis.yml Outdated

before_script:
- pip install flake8
- flake8 . --count --select=E9,F63,F72,F82 --show-source --statistics

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

@cclauss I'm not an expert on travis-cli. What is the effect of adding flake8 checks before the tests? Will the build fail if the entire framework doesn't pass flake8?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

As well, what's the justification behind the selected checks?

@cclauss cclauss Jun 27, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

There are two advantages to putting the flake8 checks in the before_script:

  1. flake8 processes to codebase really quickly and these are fast-fail tests. We want to give committers rapid feedback if we spot a typo or syntax error in their submission.
  2. Travis CI hides the output of successful before_script commands which reduces visual clutter so that committers can more rapidly find failing tests. This is especially useful flake8 --exit-zero runs but is also useful elsewhere.

On the test selection, I have no interest in "style violations" (the majority of flake8 error codes that python/black can autocorrect). Instead these tests focus on runtime safety and correctness:

  • E9 tests are about Python syntax errors usually raised because flake8 can not build an Abstract Syntax Tree (AST). Often these issues are a sign of unused code or code that has not been ported to Python 3. These would be compile-time errors in a compiled language but in a dynamic language like Python they result in the script halting/crashing on the user.
  • F63 tests are usually about the confusion between identity and equality in Python. Use ==/!= to compare str, bytes, and int literals is the classic case. These are areas where a == b is True but a is b is False (or vice versa).
  • F72 tests syntax errors in type hints
  • F82 tests are almost always undefined names which are usually a sign of a typo, missing imports, or code that has not been ported to Python 3. These also would be compile-time errors in a compiled language but in Python a NameError is raised which will halt/crash the script on the user.

@cclauss

cclauss commented Jun 28, 2019

Copy link
Copy Markdown
Author

@zeapo Can you please add separate pull requests to fix the items you mentioned above?

@garethdmm

Copy link
Copy Markdown
Owner

Hey @cclauss, I opened a python3 branch to centralize work on this. Would you mind-reopening this PR against that branch?

cclauss added a commit to cclauss/gryphon that referenced this pull request Jul 12, 2019
See garethdmm#20 for the changes required to make the tests pass.
@cclauss

cclauss commented Jul 12, 2019

Copy link
Copy Markdown
Author

#59 (on master branch) and #60 (on Python 3 branch) demonstrate that the mods in this PR apply to both Py2 and Py3.

cclauss added a commit to cclauss/gryphon that referenced this pull request Jul 12, 2019
@cclauss

cclauss commented Jul 12, 2019

Copy link
Copy Markdown
Author

Closing in favor of #60

@cclauss cclauss closed this Jul 12, 2019
@cclauss
cclauss deleted the patch-1 branch July 12, 2019 23:09
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.

3 participants