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

man: Use a context manager for opening files #4291

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

Conversation

echoix
Copy link
Member

@echoix echoix commented Sep 8, 2024

In the same line of thought of other PRs of the moment fixing ResourceWarnings of unclosed files, this also applies the same context manager pattern when used on multiple lines + removing the f.close(), or using pathlib's Path().read_text()/ Path().write_text() when the whole file is acted upon in one step.

I am seeing these when trying to make the same script that OSGeo4W uses to package grass, (since it runs the manual creation).

@echoix echoix requested a review from neteler September 8, 2024 14:45
@github-actions github-actions bot added Python Related code is in Python docs labels Sep 8, 2024
@echoix echoix added this to the 8.5.0 milestone Sep 8, 2024
@neteler
Copy link
Member

neteler commented Sep 8, 2024

Let me suggest @landam to review, also to avoid conflicts with the upcoming markdown changes in #3849

@neteler neteler requested review from landam and removed request for neteler September 8, 2024 20:00
@echoix
Copy link
Member Author

echoix commented Sep 8, 2024

Let me suggest @landam to review, also to avoid conflicts with the upcoming markdown changes in #3849

I see that some of the files have context managers in his PR, but in the meantime until that #3849 PR is ready, we still have this to handle. It will have a conflict to merge, but the diff will be smaller after.

@wenzeslaus
Copy link
Member

Given the anticipated conflicts with the Markdown doc work and the fact that the build now fails, what about just merging the per-file exclusions for now?

@echoix
Copy link
Member Author

echoix commented Sep 16, 2024

Y'a I saw these failures.. automatic merging must have messed up somewhere, as only the pyproject.toml had conflicts that I manually solved (they were hard, since the INT001,INT002,INT003 exclusions were gone).

What about creating a PR for the exclusions, and one for the parts that the markdown docs PR doesn't touch (if they are some). Because in the meantime, we see hundreds (maybe even thousands) of warnings on builds, visible on windows for example (because I was working with them). It's not any better to delay some work on the hope that an incomplete PR might be merged someday.

@wenzeslaus
Copy link
Member

I guess when the time comes for #3849 to be merged, it can replace the conflicting files with its version and go from there rather than attempting to merge the file versions together. Is that what you are suggesting @echoix?

@echoix
Copy link
Member Author

echoix commented Sep 18, 2024

I guess when the time comes for #3849 to be merged, it can replace the conflicting files with its version and go from there rather than attempting to merge the file versions together. Is that what you are suggesting @echoix?

That's a way to think about it, I didn't think exactly this way, but it can make sense

@wenzeslaus
Copy link
Member

When I locally do distclean, the subsequent build fails in man. Building again, in man or in the root dir makes the build succeed, but it is always the second build (I reproduced more than once, but didn't get to the error yet).

@echoix
Copy link
Member Author

echoix commented Sep 19, 2024

Do you happen to know what file was complaining? I'm quite surprised, as it's mostly a simple pattern applied, except for the places where we use pathlib instead, which we successfully use elsewhere. Does any change in man followed by a distclean make the second build fail (thus suggesting a problem in makefiles), or it is only with these changes specifically?

If we are unsure, I can try again from scratch, and split it in multiple PRs, one per pattern used, to know where you get this.

It's weird to me

@wenzeslaus
Copy link
Member

I get:

GISBASE="/home/vpetras/Projects/grass/code/grass/dist.x86_64-pc-linux-gnu" ARCH="x86_64-pc-linux-gnu" ARCH_DISTDIR="/home/vpetras/Projects/grass/code/grass/dist.x86_64-pc-linux-gnu" VERSION_NUMBER=8.5.0dev VERSION_DATE=2024 python3 ./build_class_graphical.py /home/vpetras/Projects/grass/code/grass/dist.x86_64-pc-linux-gnu/docs/html
touch /home/vpetras/Projects/grass/code/grass/dist.x86_64-pc-linux-gnu/docs/html/class_graphical.html
GISBASE="/home/vpetras/Projects/grass/code/grass/dist.x86_64-pc-linux-gnu" ARCH="x86_64-pc-linux-gnu" ARCH_DISTDIR="/home/vpetras/Projects/grass/code/grass/dist.x86_64-pc-linux-gnu" VERSION_NUMBER=8.5.0dev VERSION_DATE=2024 python3 ./parser_standard_options.py -t "/home/vpetras/Projects/grass/code/grass/lib/gis/parser_standard_options.c" -f "grass" -o "/home/vpetras/Projects/grass/code/grass/dist.x86_64-pc-linux-gnu/docs/html/parser_standard_options.html" -p 'id="opts_table" class="scroolTable"'
Traceback (most recent call last):
  File "./parser_standard_options.py", line 222, in <module>
    options = OptTable(parse_options(cfile.readlines(), startswith=args.startswith))
  File "./parser_standard_options.py", line 107, in parse_options
    res = parse_glines(glines)
  File "./parser_standard_options.py", line 68, in parse_glines
    key, default = (w.strip() for w in split_opt_line(line[5:]))
  File "./parser_standard_options.py", line 51, in split_opt_line
    default = default.removeprefix("_(")
AttributeError: 'str' object has no attribute 'removeprefix'
make[3]: *** [Makefile:131: /home/vpetras/Projects/grass/code/grass/dist.x86_64-pc-linux-gnu/docs/html/parser_standard_options.html] Error 1
make[3]: Leaving directory '/home/vpetras/Projects/grass/code/grass/man'
make[2]: *** [Makefile:48: default] Error 2
make[2]: Leaving directory '/home/vpetras/Projects/grass/code/grass/man'

I didn't look more into that, but that's the only "error:" in the output (not counting ctypes).

@echoix
Copy link
Member Author

echoix commented Sep 19, 2024

Ah, that should be a simple one: str always has removeprefix since it was introduced in 3.9, our minimum supported Python version for the main (dev) branch for 8.5. So you are using a not supported Python version, it's all.

@echoix
Copy link
Member Author

echoix commented Sep 19, 2024

It's not even from that PR, but from this weekend (of a PR filed 5 days ago): 560340a

It wasn't problematic as "we don't support Python 3.8" since mid June on the main branch, but was changed in the pyproject.toml only later on, that allows tools, and even black to use syntax that is only allowed in later versions. (For example the parenthesized grouped with statements in 3.9 grammar and supported, but announced in 3.10 with the new Python parser)

@echoix
Copy link
Member Author

echoix commented Sep 19, 2024

It wasn't problematic as "we don't support Python 3.8" since mid June on the main branch, but was changed in the pyproject.toml only later on, that allows tools, and even black to use syntax that is only allowed in later versions. (For example the parenthesized grouped with statements in 3.9 grammar and supported, but announced in 3.10 with the new Python parser)

I realize that I never applied that upgrade to black specifically, only ruff and other tools that respect the project-level Python version.

I drafted something out, but just need to apply one change that black prevented itself from doing since it needed to support Python 3.8:
echoix#235

@wenzeslaus
Copy link
Member

Thanks for checks. It seems I have some updates to do.

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

Successfully merging this pull request may close these issues.

3 participants