Skip to content

python: replace mutable default arguments (W0102) and enable unused wildcard import check (W0614) #6669

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

Open
RhythmP619 wants to merge 9 commits into
base: main
Choose a base branch
from
Open

python: replace mutable default arguments (W0102) and enable unused wildcard import check (W0614) #6669

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
78ef2ec
fix: replace mutable default arguments to fix Pylint W0102 warnings
RhythmP619 Nov 26, 2025
aa0f03b
fix: enabled W0614
RhythmP619 Nov 26, 2025
25dee52
Update python/grass/pygrass/messages/__init__.py
RhythmP619 Nov 26, 2025
dc6436e
Update python/grass/pygrass/messages/__init__.py
RhythmP619 Nov 26, 2025
3da5f10
doc: document None default handling for temporal lists
RhythmP619 Nov 29, 2025
2ce15a0
doc: remove trailing whitespaces
RhythmP619 Nov 29, 2025
1e8184a
Merge branch 'main' into fix-w0102-w0614
echoix Nov 29, 2025
3d58171
doc: fix doctring
RhythmP619 Nov 30, 2025
a1400af
Merge branch 'main' into fix-w0102-w0614
RhythmP619 Nov 30, 2025
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: 0 additions & 2 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -721,7 +721,6 @@ disable = [
"RP0401", # Report: Imports checker: External dependencies
"RP0801", # Report: Similarities: Duplication
"W0101", # Unreachable code (unreachable)
"W0102", # Dangerous default value %s as argument (dangerous-default-value)
"W0104", # Statement seems to have no effect (pointless-statement)
"W0106", # Expression "%s" is assigned to nothing (expression-not-assigned)
"W0108", # Lambda may not be necessary (unnecessary-lambda)
Expand All @@ -745,7 +744,6 @@ disable = [
"W0611", # (unused-import)
"W0612", # Unused variable %r (unused-variable)
"W0613", # Unused argument %r (unused-argument)
"W0614", # Unused import(s) %s from wildcard import of %s (unused-wildcard-import)
"W0621", # Redefining name %r from outer scope (line %s) (redefined-outer-name)
"W0621", # Redefining name %r from outer scope (line %s) (redefined-outer-name)
"W0622", # Redefining built-in %r (redefined-builtin)
Expand Down
13 changes: 7 additions & 6 deletions python/grass/pygrass/messages/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,10 +339,10 @@ def test_fatal_error(self, message: str) -> None:
time.sleep(1)


_MSGR_INSTANCE: Messenger | None = None


def get_msgr(
instance=[
None,
],
*args,
**kwargs,
) -> Messenger:
Expand All @@ -358,9 +358,10 @@ def get_msgr(
>>> msgr0 is msgr2
False
"""
if not instance[0]:
instance[0] = Messenger(*args, **kwargs)
return instance[0]
global _MSGR_INSTANCE
if _MSGR_INSTANCE is None:
_MSGR_INSTANCE = Messenger(*args, **kwargs)
return _MSGR_INSTANCE
Comment on lines +361 to +364
Copy link
Member

@echoix echoix Nov 26, 2025

Choose a reason for hiding this comment

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

I don't really like the approach used here. I think it shows well how the previous code was abusing the mutability of the default value, and was affected by it (it was replacing the element at index 0 probably because of that).

However, global variables are usually a source of problems, harder to reason correctly about them. Now, is it worse than the dangerous default value, probably not understood?

I don't understand extremely well the pygrass message module, so I'd like some help here

Copy link
Author

@RhythmP619 RhythmP619 Nov 26, 2025

Choose a reason for hiding this comment

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

I agree that the global variable approach can introduce complexity, and the previous code’s mutable default was indeed problematic. My intent was to quickly fix the W0102 issue with a straightforward pattern, but I see the benefit of encapsulating this in a class method or decorator for better clarity and maintainability.

I can update the code to use a class method singleton or a decorator pattern if that’s preferred.

Copy link
Contributor

@petrasovaa petrasovaa Dec 4, 2025

Choose a reason for hiding this comment

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

I don't have any special insights to it, but I think this is at least more readable than the previous code. The "instance" parameter doesn't seem to be used anywhere, so I think it's fine to get rid of it.



if __name__ == "__main__":
Expand Down
67 changes: 47 additions & 20 deletions python/grass/temporal/temporal_algebra.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1112,19 +1112,22 @@ def overlay_map_extent(
returncode = 0
return returncode

def set_temporal_extent_list(self, maplist, topolist=["EQUAL"], temporal="l"):
def set_temporal_extent_list(self, maplist, topolist=None, temporal="l"):
"""Change temporal extent of map list based on temporal relations to
other map list and given temporal operator.

:param maplist: List of map objects for which relations has been build
correctly.
:param topolist: List of strings of temporal relations.
:param topolist: List of strings of temporal relations. If None,
defaults to ["EQUAL"] internally.
:param temporal: The temporal operator specifying the temporal
extent operation (intersection, union, disjoint
union, right reference, left reference).

:return: Map list with specified temporal extent.
"""
if topolist is None:
topolist = ["EQUAL"]
resultdict = {}
temporal_topo_list, spatial_topo_list = self._check_topology(topolist=topolist)

Expand Down Expand Up @@ -1399,7 +1402,7 @@ def build_spatio_temporal_topology_list(
self,
maplistA,
maplistB=None,
topolist=["EQUAL"],
topolist=None,
assign_val: bool = False,
count_map: bool = False,
compare_bool: bool = False,
Expand All @@ -1411,7 +1414,8 @@ def build_spatio_temporal_topology_list(

:param maplistA: List of maps.
:param maplistB: List of maps.
:param topolist: List of strings of spatio-temporal relations.
:param topolist: List of strings of spatio-temporal relations. If None,
defaults to ["EQUAL"] internally.
:param assign_val: Boolean for assigning a boolean map value based on
the map_values from the compared map list by
topological relationships.
Expand Down Expand Up @@ -1573,6 +1577,8 @@ def build_spatio_temporal_topology_list(
"""
# Check the topology definitions and return the list of temporal and spatial
# topological relations that must be fulfilled
if topolist is None:
topolist = ["EQUAL"]
temporal_topo_list, spatial_topo_list = self._check_topology(topolist=topolist)

resultdict = {}
Expand Down Expand Up @@ -1624,20 +1630,25 @@ def build_spatio_temporal_topology_list(
return sorted(resultlist, key=AbstractDatasetComparisonKeyStartTime)

def assign_bool_value(
self, map_i, temporal_topo_list=["EQUAL"], spatial_topo_list=[]
self, map_i, temporal_topo_list=None, spatial_topo_list=None
) -> bool:
"""Function to assign boolean map value based on the map_values from the
compared map list by topological relationships.

:param map_i: Map object with temporal extent.
:param temporal_topo_list: List of strings for given temporal relations.
:param spatial_topo_list: List of strings for given spatial relations.
:param temporal_topo_list: List of strings for given temporal relations. If None,
defaults to ["EQUAL"] internally.
:param spatial_topo_list: List of strings for given spatial relations. If None,
defaults to empty list internally.

:return: Map object with conditional value that has been assigned by
relation maps that fulfil the topological relationships to
maplistB specified in temporal_topo_list.
"""

if temporal_topo_list is None:
temporal_topo_list = ["EQUAL"]
if spatial_topo_list is None:
spatial_topo_list = []
temporal_relations = map_i.get_temporal_relations()
condition_value_list = []
for topo in temporal_topo_list:
Expand Down Expand Up @@ -1669,22 +1680,27 @@ def compare_bool_value(
map_i,
compop,
aggregate,
temporal_topo_list=["EQUAL"],
spatial_topo_list=[],
temporal_topo_list=None,
spatial_topo_list=None,
):
"""Function to evaluate two map lists with boolean values by boolean
comparison operator.

:param map_i: Map object with temporal extent.
:param compop: Comparison operator, && or ||.
:param aggregate: Aggregation operator for relation map list, & or \\|.
:param temporal_topo_list: List of strings for given temporal relations.
:param spatial_topo_list: List of strings for given spatial relations.
:param temporal_topo_list: List of strings for given temporal relations. If None,
defaults to ["EQUAL"] internally.
:param spatial_topo_list: List of strings for given spatial relations. If None,
defaults to empty list internally.

:return: Map object with conditional value that has been evaluated by
comparison operators.
"""

if temporal_topo_list is None:
temporal_topo_list = ["EQUAL"]
if spatial_topo_list is None:
spatial_topo_list = []
temporal_relations = map_i.get_temporal_relations()

# Build conditional list with elements from related maps and given relation
Expand Down Expand Up @@ -1760,7 +1776,7 @@ def perform_temporal_selection(
self,
maplistA,
maplistB,
topolist=["EQUAL"],
topolist=None,
inverse: bool = False,
assign_val: bool = False,
):
Expand All @@ -1770,7 +1786,8 @@ def perform_temporal_selection(
expression.
:param maplistB: List of maps representing the right side of a temporal
expression.
:param topolist: List of strings of temporal relations.
:param topolist: List of strings of temporal relations. If None,
defaults to ["EQUAL"] internally.
:param inverse: Boolean value that specifies if the selection should be
inverted.
:param assign_val: Boolean for assigning a boolean map value based on
Expand Down Expand Up @@ -1828,6 +1845,8 @@ def perform_temporal_selection(
Map a4 has no equal relation to mapset mapsB

"""
if topolist is None:
topolist = ["EQUAL"]
if not inverse:
topolist = self.build_spatio_temporal_topology_list(
maplistA, maplistB, topolist, assign_val=assign_val
Expand All @@ -1849,14 +1868,15 @@ def perform_temporal_selection(
# Sort list of maps chronological.
return sorted(resultlist, key=AbstractDatasetComparisonKeyStartTime)

def set_granularity(self, maplistA, maplistB, toperator="l", topolist=["EQUAL"]):
def set_granularity(self, maplistA, maplistB, toperator="l", topolist=None):
"""This function sets the temporal extends of a list of maps based on
another map list.

:param maplistB: List of maps.
:param maplistB: List of maps.
:param toperator: String containing the temporal operator: l, r, d, i, u.
:param topolist: List of topological relations.
:param topolist: List of topological relations. If None, defaults to ["EQUAL"]
internally.

:return: List of maps with the new temporal extends.

Expand Down Expand Up @@ -1900,6 +1920,8 @@ def set_granularity(self, maplistA, maplistB, toperator="l", topolist=["EQUAL"])

:raises SyntaxError: If an unpermitted temporal relation name is used in ``topolist``
"""
if topolist is None:
topolist = ["EQUAL"]
topologylist = [
"EQUAL",
"FOLLOWS",
Expand Down Expand Up @@ -2153,14 +2175,16 @@ def eval_global_var(self, gvar, maplist):
map_i.condition_value = boolname
return maplist

def eval_map_list(self, maplist, thenlist, topolist=["EQUAL"]):
def eval_map_list(self, maplist, thenlist, topolist=None):
"""This function transfers boolean values from temporal expression
from one map list to another by their topology. These boolean
values are added to the maps as condition_value.

:param maplist: List of map objects containing boolean map values.
:param thenlist: List of map objects where the boolean values
should be added.
:param topolist: List of temporal relations between the two map lists. If None,
defaults to ["EQUAL"] internally.

:return: List of maps from thenlist with added conditional boolean values.
"""
Expand All @@ -2172,11 +2196,13 @@ def eval_map_list(self, maplist, thenlist, topolist=["EQUAL"]):
# inverse = True,
# topolist = topolist)
# Combining the selection and inverse selection list.
if topolist is None:
topolist = ["EQUAL"]
return self.perform_temporal_selection(
thenlist, maplist, assign_val=True, topolist=topolist
)

def build_condition_list(self, tvarexpr, thenlist, topolist=["EQUAL"]):
def build_condition_list(self, tvarexpr, thenlist, topolist=None):
"""This function evaluates temporal variable expressions of a conditional
expression in two steps.
At first it combines stepwise the single conditions by their relations with
Expand Down Expand Up @@ -2207,7 +2233,8 @@ def build_condition_list(self, tvarexpr, thenlist, topolist=["EQUAL"]):
:return: Map list with conditional values for all temporal expressions.

"""

if topolist is None:
topolist = ["EQUAL"]
# Evaluate the temporal variable expression and compute the temporal combination
# of conditions.

Expand Down
Loading
Loading