New functionality that cleans up a lot and makes this into a real python module#53
New functionality that cleans up a lot and makes this into a real python module#53Spatenheinz wants to merge 19 commits intoArtifactLabs:masterfrom
Conversation
This commit introduces create_rbgit which moves the logic of finding nca_path out of main as to make it easier to create RbGit instances e.g. in test cases without duplicated code. We also enable RbGit to be context managed.
This commit fixes some bugs which occured as the these parts did not change when the schema for artifact/meta-for-commit did. Also includes tests to ensure this does not happen again
bug: fix wrongful meta-for-commit after new scheme See merge request csfw/flow/git-recycle-bin!11
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Spatenheinz
force-pushed
the
master
branch
3 times, most recently
from
89763cc to
09abfbd
Compare
Spatenheinz
changed the title
eisbaw
left a comment
eisbaw
left a comment
There was a problem hiding this comment.
Thanks Jacob! Here is my (and Claude's) review.
Approved Commits
- 7d78460: Excellent Nix refactoring - improves composability with
callPackagepattern - 6e76ac3: Outstanding dict-to-dataclass refactoring - significantly improves type safety
- ea117db: Clean custom trailers feature - good separation of concerns, backward compatible
- db426b4: Critical bug fix for metadata reference construction with comprehensive tests
Bugs
- 3adecc4 & 52a6a55: Infinite recursion in
jq()function - immediate stack overflow - a1602f5: Silent failure bug - errors from
download_singlenever captured - cf309ad: Missing return statements cause AttributeError at runtime
- 088b813: Removed conditional logic breaks API contract, mutable default arguments
- 29a0650: Resource leaks - context manager only cleans up on success
- b8d9708: Broken function calls, incomplete implementation
Jacob's Likely Motivations
- Library interface focus: Converting CLI-first code into reusable library functions for programmatic use
- Batch processing efficiency: Replace one-at-a-time operations with bulk operations (e.g., download multiple artifacts)
- Dependency injection: Make RbGit objects easier to create and test by accepting external dependencies
- Better composability: Split monolithic functions into smaller, composable pieces
- Type safety improvements: Move from untyped dicts to strongly-typed dataclasses
- Query abstraction: Create flexible query system for filtering artifacts with jq integration
- Resource control: Give callers more control over when cleanup happens vs automatic cleanup
Architecture Questions
- rbgits dict: Why replace simple context managers with complex state dictionary? Original approach was cleaner and exception-safe
- Silent error handling: Multiple commits use
ignore_errors=True- violates fail-fast principles, should errors be surfaced instead? - Resource management: Several commits have cleanup issues - should we standardize on context managers throughout?
- API breaking changes: Several parameter reorderings and removals - is backward compatibility a concern?
- State complexity: New designs often introduce more state management - is this necessary for the library interface goals?
|
Great comments, I will look at fixing the issues ASAP :D. In regard to In regard to In regard to |
|
@eisbaw I have added fixed for the code comments in commits added on top, since you did not get to back if I should meld them into my previous commits or put them on top. |
This commit refactors the code to follow a common python package/module structure and exposes the public facing API in the __init__.py file
This change allow all commands if run via cmdline to clean as a "sideeffect". On the other hand it allows for better decoupling of responsibility in the library.
This commit allow us to do --all to show all artifacts across shas, not only for the current sha. This composes well with filters so we can find specifically what we need across many different shas. We also do a bit of refactoring/cleanup for the list module
This commit makes the list filters way more powerful by allowing to externally calling jq to filter on the meta data
This commit makes a better interface for working with the list command in a library. Furthermore the rewrite of queries now allow us to do conjunction queries.
This commit makes it easier to control printing of rbgit commands, as well as having better management of destuction of rbgit objects (its underlying git folder). This commit however doesnt address the problem that rbgit instances are thread unsafe, in that two processes that can use the same underlying git folder may end up in a situation like this: proc a starts --- do something --- gc / delete Proc b starts ------------ do something ---- try but fails
This commits main purpose is to restructure create_artifact_commit a little to be easier to read, and to analyze with tools. We also make sure to make push easier to use from library. I.e a more uniform interface to the other commands.
|
3 participants
This commit adds some commits from divergent Demant main branch changes.
I have a lot of stuff comming this way. I have some commits that makes list command more powerful and also I am reworking the repo to look more like a usual python module.
I am not sure if you want this to be multiple pull requests or if I should update this one.