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

Update to flake8 7.1.1. #627

Merged
merged 2 commits into from
Sep 18, 2024
Merged

Update to flake8 7.1.1. #627

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f9a617d
Update to flake8 7.1.1.
bdice Sep 17, 2024
f2e7ab8
add bokeh_sampledata dependency
AjayThorve Sep 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ repos:
# Explicitly specify the pyproject.toml at the repo root, not per-project.
args: ["--config", "pyproject.toml"]
- repo: https://github.com/PyCQA/flake8
rev: 6.0.0
rev: 7.1.1
hooks:
- id: flake8
args: ["--config=.flake8"]
Expand Down
4 changes: 2 additions & 2 deletions python/cuxfilter/charts/core/non_aggregate/core_non_aggregate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ def name(self):
chart_type = self.chart_type if self.chart_type else "chart"
return (
f"{self.x}_{self.y}"
f"{'_'+self.aggregate_col if self.aggregate_col else ''}"
f"{'_'+self.aggregate_fn if self.aggregate_fn else ''}"
f"{'_' + self.aggregate_col if self.aggregate_col else ''}"
f"{'_' + self.aggregate_fn if self.aggregate_fn else ''}"
f"_{chart_type}_{self.title}"
)

Expand Down
1 change: 0 additions & 1 deletion python/cuxfilter/charts/core/non_aggregate/core_stacked_line.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ class BaseStackedLine(BaseChart):
y_range: Tuple = None
use_data_tiles = False
y: list = []
colors: list = []
Copy link
Member

Choose a reason for hiding this comment

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

This one doesn't look right to me. I don't see self.colors getting set in __init__().

cuxfilter/python/cuxfilter/charts/core/non_aggregate/core_stacked_line.py

Line 59 in 53537fe

def __init__(

And I found at least one place that appears to rely on this attribute being defined on an instance:

cuxfilter/python/cuxfilter/charts/datashader/plots.py

Line 384 in 53537fe

class StackedLines(BaseStackedLine):

cuxfilter/python/cuxfilter/charts/datashader/plots.py

Line 394 in 53537fe

colors = colors or self.colors

What was the the flake8 error that this change addresses? Maybe there's another way we could address it, like a more specific type hint in the initialization of colors.

Copy link
Contributor Author

@bdice bdice Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

colors is also defined as a property here:

cuxfilter/python/cuxfilter/charts/core/non_aggregate/core_stacked_line.py

Lines 52 to 56 in f9a617d

@property
def colors(self):
if self.colors_set:
return list(self._colors_input)
return self.default_colors * len(self.y)

flake8 was mad that this object had both an attribute and a property with the same name, and it seemed to me that the property was the correct choice to keep. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

ahhhh my git grep -E '\.colors' wouldn't have caught it being defined with @property, should have thought of that!

Ok then yeah this is totally fine, and I agree with preferring the @property one. Thanks for explaining that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc: @AjayThorve for awareness

Copy link
Member

Choose a reason for hiding this comment

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

thanks, this seems correct, and yes property is the right one to keep

default_colors = ["#8735fb"]
box_selected_range = None

Expand Down
2 changes: 1 addition & 1 deletion python/cuxfilter/dashboard.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,7 @@ def _generate_query_str(self, query_dict=None, ignore_chart=""):

# extract string queries from query_dict,
# as self.query_dict also contains cudf.Series indices
str_queries_list = [x for x in query_dict.values() if type(x) == str]
str_queries_list = [x for x in query_dict.values() if type(x) is str]
return_query_str = " and ".join(str_queries_list)

# adding the popped value to the query_str_dict again
Expand Down
Loading