Conversation
The scan behind it had never run. `_run_in_background` kept a reference to the thread but not to the worker moved onto it, and the only other thing referring to that worker was `thread.started`, which Qt holds by weak reference for a bound method. So it was collected as soon as the caller returned: `run()` never executed, `finished` never fired, the thread sat in its event loop, and Qt aborted the process over it when the window went. The scan is now a QThread subclass, the way every other background job in the editor is written, so the object doing the work is the thread itself and there is nothing to collect out from under it. Reading the branches goes through `git` directly instead of GitPython. A `Repo` brings up long-lived `cat-file` children and thread-local state, and opening and closing that repeatedly from a background thread aborted the test suite on Windows within three runs; a subprocess that answers once and exits does not, and is what `git_cli.py` already does. A refresh while one is already running is skipped -- the toolbar is rebuilt on every language change, and the scans were stacking up. Tests cover the reading itself, which nothing had checked before, and the scan surviving its caller. Toolbar threads are now drained after every test.
The file name came from the GitHub API response, not from the user, and went straight into `Path(dest_dir) / file_name`. A name like `../evil.py` walks out of the plugins directory and an absolute one such as `C:/Windows/x.py` replaces it outright -- and plugins are imported and run, so that let the remote side choose where to write executable code. Names carrying a path are refused now. Both separators are rejected so the decision does not vary by platform: `sub\evil.py` is a path on Windows but a legal file name on Linux. Also makes `fetch_repo_tree` honour the branch it accepts. It was taking the argument and ignoring it. It now defaults to no ref, which reads the repository's default branch, so one whose default is called master still works.
`show_diff_of_commit` called `commit.diff(parent)`. That reads as going from the commit to its parent, which is the inverse of what the commit did: every added line was printed as a removal and every removal as an addition. The first commit, compared against the empty tree the same way, came out as the deletion of everything it introduced. Both now diff from the earlier side to the commit, and match what `git show` prints for the same revision.
`write_file` logged the whole file it was about to save. Nothing reached the log at the level shipped, but the f-string built a copy of every file on every save regardless, and one lowered level away it was writing the user's files into JEditor.log. It records the path and a length now, as `write_file_with_encoding` already did. Two modules also reconfigured logging for the entire process at import: the logging module put the root logger at DEBUG, and the git tree widget called `basicConfig`. JEditor is embedded in other applications, so both decided the log level for a host that never asked. Neither did anything for the editor -- its own logger sets its own level, and records travelling up are filtered by each handler's level, not by root's -- and the logger `basicConfig` was set up for had no callers. Also chains the two exceptions in the file helpers that were re-raised bare, losing the original cause.
Every QFontDatabase member is static in Qt6 and constructing one is deprecated; the two font menus built an instance to call `families()` on, and the main window carried one as an attribute for them. They call it on the class now and the attribute is gone, which also drops the deprecation warning from the test run. One `exec_()` left over from Qt5 becomes `exec()`. Three handlers swallowed their exception without a word: the toolbar's branch scan, jedi completion, and the binary-file sniff. Carrying on is right in all three -- an empty branch box, no completions, a file treated as text -- but with nothing recorded there was no way to find out why. They log now, at a level matching how routine each one is, and the sniff catches OSError rather than everything.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 51 |
| Duplication | 1 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Calling git by name so PATH finds it, and running this interpreter on a literal defined in the same file, are both intentional. Each call site says why and carries the suppression. Also renames a test constant that read as a credential to the analysers. It holds a line of text a save must not copy into the log, and now says so.
A comma separated list after nosec is not the documented syntax and only the first id took effect, so the second check kept reporting.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A scan of the project turned up five defects; each commit is one of them.
The toolbar's git branch box never filled.
_run_in_backgroundkept areference to the background thread but not to the worker moved onto it, and
thread.started— the only other thing referring to that worker — is held byweak reference for a bound method. The worker was collected as soon as the
caller returned, so
run()never executed,finishednever fired, and thethread sat in its event loop until Qt aborted the process over it. It is now a
QThread subclass, matching how every other background job here is written.
Reading the branches also moved off GitPython onto
gitdirectly. ARepobrings up long-lived
cat-filechildren and thread-local state; opening andclosing that from a background thread aborted the suite on Windows in three
runs out of three, while the subprocess form passes three out of three.
A downloaded plugin could land anywhere. The file name came from the
GitHub API response and went straight into
Path(dest_dir) / file_name, so../evil.pywalked out of the plugins directory andC:/Windows/x.pyreplaced it — and plugins are imported and run.
Commit diffs were printed backwards.
commit.diff(parent)is the inverseof what a commit did; additions showed as removals. Now verified against what
git showprints.The editor logged file contents and reconfigured logging for its host.
write_filebuilt a copy of every saved file for a log line, and two modulesset root logging at import — which decides the level for whatever application
embeds JEditor.
Deprecated Qt calls and silent exception handlers are cleared out.
Verified with
ruff checkclean and the full suite run three times: 1859passed each time, against a 1819-test baseline confirmed stable over three
runs beforehand.