ciq_tag: lib and command line tool to operate on CIQ commit tags #48
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.
| 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.
ciq_tag.py
Outdated
|
|
||
| DEFAULT_LOGLEVEL = "INFO" | ||
|
|
||
| LOGLEVEL = os.environ.get( |
There was a problem hiding this comment.
I would use this as a param to the tool (the logging level)
There was a problem hiding this comment.
This would limit the log setting ability to the CLI tool only. What abut python modules which would be including this one?
Some switches might be provided, but the modules would have to be aware of them and consciously propagate end-user's logging level wish down to the library.
Environment variables cross the application-library barrier effortlessly, so the module's users don't have to deal with these settings. For example, a ciq_tag_user.py program may look like
#!/usr/bin/env python3
import ciq_tag
print(ciq_tag.add_tag("""
sunrpc: handle SVC_GARBAGE during svc auth processing as auth error
tianshuo han reported a remotely-triggerable crash if the client sends a
kernel RPC server a specially crafted packet. If decoding the RPC reply
fails in such a way that SVC_GARBAGE is returned without setting the
rq_accept_statp pointer, then that pointer can be dereferenced and a
value stored there.
""", ciq_tag.CiqTag.CVE, "CVE-2025-38089"))
and calling it as
LOGS=DEBUG ./ciq_tag_user.py
would show debug messages from the ciq_tag library without engaging ciq_tag_user.py in it.
If the programs using ciq_tag wished they may "plug" themselves into this LOGS variable logic as well, of course, so they recognize the logging level wanted by the end user in the same way. It is a form of very simple global logging setup, generally advised for python logging.
Having said that, the logging setup I did was really poor and lazy. Rewrote it.
| dedent_text, | ||
| value)) | ||
|
|
||
| # Elementary operations ############################################################################ |
There was a problem hiding this comment.
I would add comments inline the function with docstring.
There was a problem hiding this comment.
I would add the docstrings, but that's a bit of work. I wanted to see first if it's even worth it. Do you mind if we wait for others from the team to see if they even want this lib?
| return (True, (message[:deleted_tag_pos.keyword_start] + | ||
| omit_prefixing_empty_lines(message[deleted_tag_pos.boundary_end:]))) | ||
| else: | ||
| return (False, message) |
There was a problem hiding this comment.
I haven't checked this properly, but my personal preference is to avoid multiple indents and do early returns if possible. What do you think?
There was a problem hiding this comment.
I guess we both want the code to look good. My criteria for code aesthetic is based on how well it exposes the behavioral complexity of an algorithm. Nested if/else statements make the task of answering key questions like "what are the conditions for function f to return value y?" as simple as taking the conjunction of conditions up the branching hierarchy from the picked exit point, while the existence of early returns necessitate mental code execution, which is prone to errors, takes more time and unnecessary effort.
This particular code also doesn't deal with eliminating error cases which the early returns seem to be most suited for.
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. |
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