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

Fix Flake8 errors and improve exception handling in lib/init/grass.py. Fixes ResourceWarning for python/libgrass_interface_generator/ctypesgen/version.py #4285

Closed
wants to merge 3 commits into from
Closed

Fix Flake8 errors and improve exception handling in lib/init/grass.py. Fixes ResourceWarning for python/libgrass_interface_generator/ctypesgen/version.py #4285

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ad724bb
Update .flake8 configuration
arohanajit Sep 6, 2024
5a0cd7d
asdasd
arohanajit Sep 6, 2024
3607dbc
Resource Warning
arohanajit Sep 6, 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
1 change: 0 additions & 1 deletion .flake8
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ per-file-ignores =
# F841 local variable assigned to but never used
# E741 ambiguous variable name 'l'
__init__.py: F401, F403
lib/init/grass.py: E722, F821, F841
utils/gitlog2changelog.py: E722, E712
man/build_check_rest.py: F403, F405
man/build_full_index_rest.py: F403, F405
Expand Down
10 changes: 5 additions & 5 deletions lib/init/grass.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,7 +484,7 @@ def create_gisrc(tmpdir, gisrcrc):
if "UNKNOWN" in s:
try_remove(gisrcrc)
s = None
except:
except Exception:
s = None

# Copy the global grassrc file to the session grassrc file
Expand Down Expand Up @@ -1176,7 +1176,7 @@ def set_language(grass_config_dir):
encoding = "UTF-8"
normalized = locale.normalize("%s.%s" % (language, encoding))
locale.setlocale(locale.LC_ALL, normalized)
except locale.Error as e:
except locale.Error:
if language == "en":
# A workaround for Python Issue30755
# https://bugs.python.org/issue30755
Expand All @@ -1202,7 +1202,7 @@ def set_language(grass_config_dir):
# See bugs #3441 and #3423
try:
locale.setlocale(locale.LC_ALL, "C.UTF-8")
except locale.Error as e:
except locale.Error:
# All lost. Setting to C as much as possible.
# We can not call locale.normalize on C as it
# will transform it to en_US and we already know
Expand Down Expand Up @@ -1585,7 +1585,7 @@ def say_hello():

revision = linerev.split(" ")[1]
sys.stderr.write(" (" + revision + ")")
except:
except Exception:
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this add add another warning, where we don't use anything in that except?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per flake8, there should be no bare 'except' statements E722 - and at the same time it shouldn't be assigned to an unused variable which will trigger F841. I ran flake8 post this update and it didn't highlight anything

pass


Expand Down Expand Up @@ -1951,7 +1951,7 @@ def print_params(params):
try:
revision = linerev.split(" ")[1]
sys.stdout.write("%s\n" % revision[1:])
except:
except Exception:
sys.stdout.write("No SVN revision defined\n")
elif arg == "version":
sys.stdout.write("%s\n" % GRASS_VERSION)
Expand Down
16 changes: 7 additions & 9 deletions python/libgrass_interface_generator/ctypesgen/version.py
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

What do we do here, as ctypesgen is code that is supposed to be upstream? @nilason or @wenzeslaus ?

Original file line number Diff line number Diff line change
Expand Up @@ -23,18 +23,17 @@


def read_file_version():
f = open(VERSION_FILE)
v = f.readline()
f.close()
with open(VERSION_FILE) as f:
v = f.readline()
return v.strip()


def version():
try:
args = {"cwd": THIS_DIR}
devnull = open(os.devnull, "w")
p = Popen(["git", "describe"], stdout=PIPE, stderr=devnull, **args)
out, err = p.communicate()
with open(os.devnull, "w") as devnull:
p = Popen(["git", "describe"], stdout=PIPE, stderr=devnull, **args)

Check notice

Code scanning / Bandit

Starting a process with a partial executable path Note

Starting a process with a partial executable path
out, err = p.communicate()
if p.returncode:
raise RuntimeError("no version defined?")
git_tag = out.strip().decode()
Expand All @@ -60,9 +59,8 @@
def write_version_file(v=None):
if v is None:
v = version()
f = open(VERSION_FILE, "w")
f.write(v)
f.close()
with open(VERSION_FILE, "w") as f:
f.write(v)
Comment on lines +62 to +63
Copy link
Member

Choose a reason for hiding this comment

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

Ruff should suggest you to change it in a single Pathlib write_text call, with FURB103 rule

Copy link
Contributor Author

@arohanajit arohanajit Sep 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@echoix I was going off of ResourceWarnings so I hadn't checked the Ruff outputs. I ran them just now - there were a lot of SIM115 but no FURB103 in ctypesgen

Copy link
Member

Choose a reason for hiding this comment

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

I think you need the --preview flag.

Or better idea, I had it excluded in config maybe, so maybe either give the path on the CLI, or use something like --isolated to ignore all configuration files

Copy link
Contributor Author

@arohanajit arohanajit Sep 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah I see it now. Either way updating this PR would become too messy. I'll close this PR and clone a clean copy and create separate PR for these 2 warnings. Thanks!



VERSION = version()
Expand Down
Loading