WIP: introduce the cache-files check - #7
Conversation
enr0n
left a comment
enr0n
left a comment
There was a problem hiding this comment.
Thanks for the PR! This will be a great addition. This was a quick review so please ask if anything I suggested did not make sense.
Please also consider adding some tests for this.
| # XXX this needs to be better defined | ||
| # It's currently needed to be able to extract the .dsc in the | ||
| # cache-files check, but that extraction should probably be handled | ||
| # more generally in that Context class. | ||
| self._changes_path = changes |
There was a problem hiding this comment.
I think it makes sense to make .dsc an attribute of Context, just like the changes file, source dir, etc. You can add some sensible defaults to try and derive it by default (see the logic for changes file).
There was a problem hiding this comment.
In the end, when implementing the "transparent access" that jj suggested, I didn't need the .dsc anymore, only the path to the .changes, so I implemented properly that property, and the dput entry-point is making use of it.
I guess this could spark further refactor where we could actually only provide the path, and the Context would then go read it to populate Context.changes, but I believe that would belong to some subsequent refactor. What do you think?
b7540b4 to
6ea1475
Compare
| for cache_file in [ | ||
| # General | ||
| "CACHEDIR.TAG", | ||
| ".direnv", | ||
| "/.git/", # XXX: that one is kind of a problem when `ubuntu-lint` is invoked as a CLI in a git repo. Maybe we should see to activate that only for the `dput` hook? | ||
| # Python | ||
| ".pyc", | ||
| ".egg-info", | ||
| ".tox", | ||
| "__pycache__", | ||
| ]: |
There was a problem hiding this comment.
to avoid reconstructing them and for faster lookups, this bad-name list could be defined outside the _get_bad_files function as a set. and there should be a easy way to extend this list, for example to add ruff_cache or mypy_cache files.
There was a problem hiding this comment.
By "easy way to extend", do you mean a configuration file of sorts? If so, that's probably out of scope for now, but I would eventually like to have something like that.
There was a problem hiding this comment.
yea, but we should at least add more of the common cachefiles, like mypy_cache, pytest_cache, maybe even CACHEDIR.TAG files.
.direnv and .envrc are often in my devtrees but should probably not end up in a package.
| def __init__( | ||
| self, | ||
| changes: str | deb822.Changes | None = None, | ||
| changes_path: str | Path | None = None, |
There was a problem hiding this comment.
Hm, this feels a bit redundant since changes already accepts a path (though maybe the type annotation should be updated to make that more clear).
I think I would rather do one of the following:
- Add a
filesparameter to context, and pass those paths directly in the dput hook withchanges.get_files(). Or; - Keep the
changes_pathproperty, but set it whenchangesis a path, and not raw data. Then, in the dput hook start passing the absolute file path withchanges.get_changes_file()instead of passing the raw data.
What do you think?
There was a problem hiding this comment.
Or maybe we want a debian_tar parameter instead. I am working on another lint where this would be helpful, so I might work on this part of things separately.
| try: | ||
| # Case where we directly have the source_dir | ||
| files = [str(p) for p in Path(context.source_dir).absolute().glob("**")] | ||
| print(files) | ||
| bad_files += _get_bad_files(files) | ||
| except MissingContextException: | ||
| pass | ||
|
|
||
| try: | ||
| # Case of a .changes file and we need to read the tarballs | ||
| changes_files = context.changes.get("Files") | ||
| if changes_files: | ||
| for item in changes_files: | ||
| if ".tar." in item["name"]: | ||
| file_path = context.changes_path.parent / item["name"] | ||
| with tarfile.open(file_path, "r") as tar: | ||
| bad_files += _get_bad_files(tar.getnames()) | ||
| except MissingContextException: | ||
| pass |
There was a problem hiding this comment.
Shouldn't these be mutually exclusive cases? Or, I guess there needs to be some consistency check as is done in other places where data can come from multiple context sources.
| pass | ||
|
|
||
| if bad_files: | ||
| context.lint_warn( |
There was a problem hiding this comment.
This should be lint_fail(). The callers can decide to downgrade or not. I.e. I see in the CLI you configured this to be warning by default.
| for cache_file in [ | ||
| # General | ||
| "CACHEDIR.TAG", | ||
| ".direnv", | ||
| "/.git/", # XXX: that one is kind of a problem when `ubuntu-lint` is invoked as a CLI in a git repo. Maybe we should see to activate that only for the `dput` hook? | ||
| # Python | ||
| ".pyc", | ||
| ".egg-info", | ||
| ".tox", | ||
| "__pycache__", | ||
| ]: |
There was a problem hiding this comment.
By "easy way to extend", do you mean a configuration file of sorts? If so, that's probably out of scope for now, but I would eventually like to have something like that.
| # Case of a .changes file and we need to read the tarballs | ||
| changes_files = context.changes.get("Files") | ||
| if changes_files: | ||
| for item in changes_files: | ||
| if ".tar." in item["name"]: | ||
| file_path = context.changes_path.parent / item["name"] | ||
| with tarfile.open(file_path, "r") as tar: |
There was a problem hiding this comment.
With #18, you should be able to just do:
with tarfile.open(context.debian_tar, "r:*") as tar:
...3 participants
This is a port of that original MP.
Please have a look at all the
XXXcomments, they contain questions that need to be answered before even thinking about merging this.