ciq_tag: lib and command line tool to operate on CIQ commit tags#48
ciq_tag: lib and command line tool to operate on CIQ commit tags#48pvts-mat wants to merge 3 commits intoctrliq:mainlinefrom
Conversation
|
No docs in the code - will be added if the tool finds acceptance. Same for the usage examples from the library POV |
pvts-mat
force-pushed
the
commit-msg-lib
branch
from
034fc8d to
cc1a2f0
Compare
|
I am not sure how often I would use this tool, except for maybe some mistakes like modifying the upstream-diff comment later. The rest of the tags should be automated. But, I think the library would be very useful to improve the current tooling. Can you separate the library from the CLI tool to make things easier? |
Add this as dependency in pyproject.toml. As explained here. I realized there's no readme with this, will add this soon. And solve the formatting error in the pr_check. |
roxanan1996
left a comment
roxanan1996
left a comment
There was a problem hiding this comment.
Will continue reviewing and testing tomorrow, but first the formatting has to be addressed.
ciq_tag.py
Outdated
| args = read_args() | ||
| for c in Parameter: | ||
| logging.debug(f"{c}: {c.get_val(args)}") | ||
| return CommandRoot.get_by_cmd(args.CommandRoot).process(args) |
There was a problem hiding this comment.
Any reason why this was implemented from scratch? Why not use argparse or even better click (https://click.palletsprojects.com/en/stable/)
I want to change all argparse usages to click, but haven't had the time.
There was a problem hiding this comment.
It is argparse. I didn't implement anything from scratch that wasn't implemented by argparse already. What I implemented was a translation from flexible data structures to inflexible argparse procedure calls.
With regards to click I didn't know about this tool. Maybe it provides what I needed, maybe not. I would have to evaluate it.
pvts-mat
force-pushed
the
commit-msg-lib
branch
from
cc1a2f0 to
54142e6
Compare
|
Thanks for looking into it @roxanan1996. Let me address the comments
The library was the main goal. I wanted to create an abstraction layer over CIQ meta-data operations, hoping to reduce code duplication, wheel reinvention and the associated mental load. The development of the command line tool wasn't driven by some expected use cases, but rather guided by the principle of computational freedom. The aim was to transfer 1-to-1 the functionality of the library to the computation environment most natural and default for many tasks - the shell. I try not to mold the developed tools into imagined usecases because my imagination is limited and I do not wish to impose it on others. Regarding the typical commit backporting workflow I don't think the tool is much useful, but it does open the door to automation of many kinds, which may be very useful in larger PRs like the recent Additionaly the CLI frontend is very useful in testing the library, so I would have written it anyway.
You mean the "things" here are the decision about what to keep and what to reject? Because if it's about usage then these two do not conflict in any way, you can |
Done. |
Yeah, completely agree, that's what I meant. It will help improving the existing tooling a lot. As for the separation, I think it's usually best practice to separate the lib and the tool, if we need to import the lib in another tool we have. I am 100% sure the lib would be used further, so it's just easier to do separate them now then in the future. |
|
I am a bit busy today. but I'll continue looking at this tomorrow. |
- Rewrote the tags parsing logic. The previous one was based on the on-demand, separate, atomic message parsing for each tag operation. It was kinda raw and liberal, avoiding building additional data structures. The goal was to keep it flexible, but it turned out to be too flexible, recognizing parts of upstream message as CIQ tags, like in the case of the commit 081056dc00a27bccb55ccc3c6f230a3d5fd3f7e0. In the new version the CIQ tags are required to consitute a continuous block at the very beginning of git message. The message is parsed in whole, all tags tracked no matter what operation on which of them is requested. An explicit list of tags for the given commit message is build, which allows for simplification of a lot of editing code. From the API perspective there is no longer a loose bag of functions but a CiqMsg class encapsulating the operations as modifier methods, with the `get_message()' getter method to obtain the end result of all carried out operations (adding/editing/deleting tags). The command line interface remained exactly the same. - Extracted the CLI to a separate file ciq_tag.py: library ciq-tag.py: executable CLI - Rewrote arguments parsing from argpars to click
pvts-mat
force-pushed
the
commit-msg-lib
branch
from
bea0b3b to
a269fd3
Compare
|
roxanan1996
left a comment
roxanan1996
left a comment
There was a problem hiding this comment.
hi. Sorry it took so long.
I left some very minor comments.
Overall it looks ok to me and it would be useful to add it.
People can iterate over it once it gets used.
So feel free to push it.
ciq_tag.py
Outdated
| UPSTREAM_DIFF = (["upstream-diff"], True) | ||
|
|
||
| def __init__(self, keywords: List[str], multiline: bool = False): | ||
| assert len(keywords) > 0 |
There was a problem hiding this comment.
I should return an Exception here or sth, I use assert only for tests.
There was a problem hiding this comment.
If this particular condition is false it means the enum entry was defined wrong, so this assert is more of a buggy code canary than an error path of a regular program execution. Switched the asserts to exceptions though, if you prefer.
While we are at it - how much arguments checking do you want for the public functions? Currently there is almost none. All of them? Only the least obvious? Do you want the types checked too or just values?
There was a problem hiding this comment.
I see, then the assert can be removed in this case right?
Good question about checking. Ideally, as much as possible. Your call.
|
|
||
|
|
||
| def process_in_out(input, output, result_to_output_map, ciq_msg_method, *method_args_pos, **method_args_key): | ||
| with open_input(input) as in_file: |
There was a problem hiding this comment.
nitpick, input does not have to stay open after input_str is read.
|
2 participants
About
A library and a command line tool to get, insert, modify and delete CIQ tags from commit messages.
Features:
Pure python 3
Minimal dependencies
(can be avoided if required)
Tools-indepentent
gitunderneath (or any other external commands for that matter).Robust
cve-bf,cve-bugfix,cve-update, …):)upstream-diffregardless of their formatting and indentation.Handling the formatting
Full-featured
upstream-diff)jiratags).Extendable.
By encapsulating the tags editing operations this tool allows for automating those revision history maintenance tasks which previously had to be done by hand or with the use of ad-hoc
grepsandseds..Examples
Adding tags
Input
Output
Automatic tracking of the proper tags order
(
jiratag should be beforecve)Adding multiple instances of the same tag if necessary
Modifying existing tags
Input
Output
Return nonzero exit code if tag not found
(but produce the output unchanged)
Use
setto insert anyway if not existThis differs from
addwhich would always insert the tag, whilesetfirst attempts to modify the existing one and only if that didn't succeed - to insert the new instance.Deleting tags
Input
Basic usage
Handling the multi-line properties well
Clean all metadata
Shring the vertical separation if needed
Getting the values of tags
Input
Basic usage
Return error if tag not found
Extract the raw
upstream-diffAvoid the indentation
Unwrap the text to abstract from formatting where it get in the way
Handle also other multiline properties
Input:
Output:
Autoformatting of values
Input
Set
upstream-diffright from the command lineSet indentation to a fixed value
Set indent to align with the keyword
Control the width