-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Lint Python code for undefined names #1721
Changes from 3 commits
128a178
06b5c9d
a416d12
26d8600
a819e9c
8a65038
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -94,5 +94,9 @@ matrix: | |||||||||
- language: python | ||||||||||
python: "3.7" | ||||||||||
env: TOXENV=py37 | ||||||||||
dist: xenial # required for Python >= 3.7 | ||||||||||
script: *1 | ||||||||||
- name: "Lint Python code with flake8" | ||||||||||
language: python | ||||||||||
python: "3.7" | ||||||||||
install: pip install flake8 | ||||||||||
script: flake8 . --count --exclude=./.*,*/visualization/tfdv.py --select=E9,F63,F7,F82 --show-source --statistics | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For consistency can we either include roc_curve.py within this exclude statement or add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. roc_curve.py passes the flake8 tests. Why would we want to move it to this exclude statement? We only moved tfdv.py to this exclude statement because its pytests are overzealous in that they complain if a comment is added to the file. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Previously the tests failed before There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we add # flake8: noqa TODO to the top of tfdv.py then its pytest fails. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah I realize the issue now, it is due to the snapshots being incorrect, they do not include the comment. We can update the snapshots if we add # flake8: noqa TODO to the top of tfdv.py. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As a follow up pipelines/backend/src/apiserver/visualization/test_exporter.py Lines 59 to 62 in 94aa078
can be changed to use test.py rather than tfdv.py at line 61. This would prevent the overzealous unit tests from getting in the way of future changes to tfdv.py while also ensuring create_cell_from_file works as intended. After this is taken care of I think this is ready to be merged. Thanks for all your work @cclauss!
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,8 @@ | |
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
# flake8: noqa TODO | ||
|
||
import json | ||
from pathlib import Path | ||
from bokeh.layouts import row | ||
|
@@ -34,7 +36,7 @@ | |
# trueclass | ||
# true_score_column | ||
|
||
if "is_generated" is not in variables or variables["is_generated"] is False: | ||
if not variables.get("is_generated"): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that we should revert this change and move it to a separate PR. What are your thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That change has been in #1878 for 4 days (a long time for a syntax error) and was not acted on so I bundled it it here |
||
# Create data from specified csv file(s). | ||
# The schema file provides column names for the csv file that will be used | ||
# to generate the roc curve. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this no longer necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct.