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.

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

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

Conversation

arohanajit
Copy link
Contributor

@arohanajit arohanajit commented Sep 6, 2024

Description

This pull request addresses several Flake8 warnings and improves exception handling in the GRASS GIS initialization script. The changes focus on removing unused variables and replacing bare except clauses with more specific exception handling. This pull request also addresses the recurring ResourceWarning about an unclosed file in the version.py script.

Changes

  1. Removed unused 'e' variables in exception handling for locale errors.
  2. Improved exception handling when reading the VERSIONNUMBER file.
  3. Modified the version() function to use a context manager for handling the file opened for /dev/null, ensuring proper closure.
  4. Updated read_file_version() and write_version_file() functions to also use context managers for file operations.

Also as @echoix mentioned here: #4239 (comment)
workign through the errors in the result will fix most of ResourceWarnings although it also highlights simple linting cases where the warning may not exist

Specific Changes

1. Removed unused 'e' variables:

In the locale setting section, removed as e from two except locale.Error clauses where the error variable was not being used.

# Before
except locale.Error as e:
    # code not using 'e'

# After
except locale.Error:
    # code remains the same

@github-actions github-actions bot added Python Related code is in Python libraries labels Sep 6, 2024
@arohanajit arohanajit changed the title Fix Flake8 errors and improve exception handling in lib/init/grass.py Fix Flake8 errors and improve exception handling in lib/init/grass.py. Fixes ResourceWarning for python/libgrass_interface_generator/ctypesgen/version.py Sep 6, 2024
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
@@ -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

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 ?

Comment on lines +62 to +63
with open(VERSION_FILE, "w") as f:
f.write(v)
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

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

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libraries Python Related code is in Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants