Project cleanup#27
Conversation
Signed-off-by: Enji Cooper <ngie@FreeBSD.org>
pysqlite3 is a defunct project now: redirect others to install the py-sqlite3 package instead, i.e., the module that's distributed with the python language distribution. Signed-off-by: Enji Cooper <ngie@FreeBSD.org>
This ignores a lot more autogenerated paths which should not be checked in to :master. Signed-off-by: Enji Cooper <ngie@FreeBSD.org>
- Run `I` and `UP` category fixers and resolve all issues not handled via `--unsafe-fixes`. - Run through the remaining safe fixers under the `ALL` meta-category. Signed-off-by: Enji Cooper <ngie@FreeBSD.org>
|
|
Signed-off-by: Enji Cooper <ngie@FreeBSD.org>
Signed-off-by: Enji Cooper <ngie@FreeBSD.org>
- Unpack iterables instead of appending one iterable to another iterable (RUF005). - Don't assign values via mutable arguments (dicts, lists) in functions. - Fix a potential SQLite3 injection attack via an f-interpolated string. - Re-raise exceptions using `raise` when possiible to avoid mangling the traceback stack. Signed-off-by: Enji Cooper <ngie@FreeBSD.org>
markjdb
left a comment
There was a problem hiding this comment.
Please don't reformat all of the code, it makes history unreadable for no benefit and gives me a bunch of work to do when rebasing local WIP patches. I don't want to take these "chore" patches.
If you want to make the code compliant to some linter or type checker, please at least include a script so that others can easily lint their changes.
| """) | ||
| WHERE tr.result_type = ? | ||
| """, | ||
| (restype.value,), |
There was a problem hiding this comment.
There is no "attack": restype's value is defined within the code itself, and the user invoking bricoler already has control over the db file. Please don't use such language for benign things.
There was a problem hiding this comment.
I realize the wording is potentially inflammatory: it should state that the new code applies a best practice and the old code was prone to an SQL injection attack in general, but the code itself wasn't vulnerable due to how it's designed.
| "-o", | ||
| f"poolname={zfs_pool_name}", | ||
| "-o", | ||
| f"bootfs={zfs_pool_name}", |
There was a problem hiding this comment.
I find this less readable than before.
There was a problem hiding this comment.
# fmt: skip could avoid that kind of reformatting. I'll save that for when I get to that part of this larger diff.
That's a fair ask. |
It makes life more difficult when contributing back changes to improve the tool though when there isn't a baseline/automation bar being applied to bricoler.
#29 and #30 whittle away at this work a bit, but this project is definitely deserving of tests (#30 breaks initial ground, but a lot of coverage areas are missing). I see bugs in the code as it exists today -- if more tests existed, I feel that some of the issues would be more apparent/more quickly addressed. |
Reformatting all of the code doesn't apply a "baseline/automation bar" though? For now I prefer to prioritize not making my own life more difficult. Bug fixes, new features are welcome, "cleanup" not so much.
What bugs are you referring to? |
Right now contributors are required to handle reformatting/linting/etc on their own, which is extremely error prone and results in unnecessary debating over style and such. Leveraging tools like black, ruff, etc, allow you (and the project as a whole) to focus on what the code does more than how it's presented, and helps avoid defects by leveraging tools which find bugs for you.
65e3505 is the first one I've found, but one can argue that 2e0b6fe is also a bug since it mangles the traceback stack. The latter is less of an issue with python 3 thanks to exception chaining, but it was definitely a problem with python 2; I received a few defects back in the day at another job due to an issue like this (turns out the real defect was in another piece of code). |
At this point it's better to minimize dependencies. If the project gets big enough, things like this will make more sense. For now I'd like to keep it simple(r).
Both of these bugs are harmless. |
The former bug prevents makefs from actually running. Is it really harmless?
The first bug looks very valid since |
No it does not. Yes.
This getopt() call isn't even looking at the "-oversion=2" string, for two reasons: you're passing argc==0, and optind is 1 by default. So it's immediately at the end of the argv, and it returns -1 to signal that. |
Yeah, I see where I botched that example now. I'll just close this PR and #30 . I'll leave #29 to you as to whether or not you want to take that PR. |
ruff format.