Fix suboptimal file replacement#348
Conversation
dmarteau
force-pushed
the
fix-suboptimal-replace-in-file
branch
from
3d70154 to
1098843
Compare
3nids
left a comment
3nids
left a comment
There was a problem hiding this comment.
Great move!
One question is the removal of the DEBUG variable change.
| # replace any DEBUG=False in all Python files | ||
| if not is_prerelease: | ||
| for file in glob(f"{parameters.plugin_path}/**/*.py", recursive=True): | ||
| replace_in_file(file, r"^DEBUG\s*=\s*True", "DEBUG = False") |
There was a problem hiding this comment.
May be I'm wrong but I checked for DEBUG global variables in source files and found none: so the code above has no effects.
There was a problem hiding this comment.
It's checking in Python files of the plugin it self, so it depends of the plugin source code.
https://github.com/opengisch/qgis-plugin-ci?tab=readme-ov-file#debug
It's indeed missing from the HTML documentation
There was a problem hiding this comment.
Oh, I see. What is the purpose of this variable ?
IMHO this is an anti-pattern to use litteral global variables as check for DEBUG state. I think this should discouraged. Moreover this is also error prone as you assuming that plugin may defines this variables for other purposes because the name is too generic.
I may put back this code for the sake of retro compatibility, but you really should considering not relying on it or
disabling it in some way or renaming in to something less ambiguous: i.e PLUGIN_CI_DEBUG.
dmarteau
force-pushed
the
fix-suboptimal-replace-in-file
branch
from
8d8433f to
fca730d
Compare
for more information, see https://pre-commit.ci
Gustry
left a comment
Gustry
left a comment
There was a problem hiding this comment.
Nice move indeed, thanks
| f"commitNumber={len(list(repo.iter_commits()))}", | ||
| ) | ||
|
|
||
| metadata.set("general", "commitNumber", str(sum(1 for _ in repo.iter_commits()))) |
There was a problem hiding this comment.
commitNumber commitSha1 and dateTime are not necessarily in the general section because they are not "official" items from a metadata.txt according to QGIS documentation https://docs.qgis.org/3.40/en/docs/pyqgis_developer_cookbook/plugins/plugins.html#metadata-txt
It's up to the PyQGIS developer.
https://opengisch.github.io/qgis-plugin-ci/usage/cli_release.html#additional-metadata
3 participants
The 'replace-in-file' calls for modifying metadata is suboptimal: the file is read and written back for each changes in metadata properties.
The actual parsing deos not takes the sections into account: this is error prone if there is properties of the same name in different sections.
Metadata files can be handled with 'ConfigParser' in a simpler way without resorting to custom parsing