Feat/add syntax autogeneration#1980
Conversation
kkedziak-splunk
left a comment
There was a problem hiding this comment.
Review Summary
This PR adds automatic syntax generation for custom search commands based on argument definitions. This is a nice quality-of-life improvement that reduces the need for users to manually specify syntax strings.
Issues found:
-
Chain of
ifstatements instead ofelif(create_searchbnf_conf.py): The validator-type matching logic uses a series of independentifstatements instead ofelif. This means every condition is checked even after a match. For example, ifvalidator == "Integer", the firstifmatches, but thenvalidator == "Boolean",validator == "Set", etc. are all still evaluated. Worse, theparam_syntaxvariable could be overwritten if a validator name accidentally matched multiple conditions. Should useeliffor correctness and performance. -
Floatmapped to<int>in syntax (create_searchbnf_conf.py): The syntax generation mapsFloatto<int>— this is misleading.Floatvalidator should produce<float>or<number>in the syntax, not<int>. -
Durationmapped to<int>(create_searchbnf_conf.py): Similarly,Durationvalues are mapped to<int>but durations typically have formats like5m,1h— consider<duration>instead. -
syntaxno longer required (global_config_validator.py): The validation was changed to only requiredescriptionandusagewhenrequiredSearchAssistantis True, removingsyntaxfrom the required set. However the error message still says"One of the attributes among description, usage, syntax". The error message should be updated to match the new requirement. -
Magic number 120/100 for line wrapping (
create_searchbnf_conf.py): The syntax line-wrapping uses hardcoded thresholds of 120 and 100 characters. These should be named constants or at least documented in a comment explaining why these values were chosen. -
maxLength: 100removed from syntax schema (schema.json): ThemaxLengthconstraint was removed from thesyntaxfield. Since auto-generated syntax can exceed 100 chars (hence the line-wrapping logic), this makes sense, but it also means manually provided syntax strings are no longer validated for length. Was this intentional?
The feature is well-designed overall and the test coverage is good, especially the auto-generation test case with mixed validators.
| "List", | ||
| "Duration", | ||
| "Map", | ||
| ): |
There was a problem hiding this comment.
Bug: This chain of if statements should be elif. Currently, every condition is evaluated even after a match, and param_syntax could be silently overwritten. For example:
if validator in ("Integer", "Float", "Duration"):
param_syntax = ...
elif validator == "Boolean":
param_syntax = ...
elif validator == "Set":
...Without elif, this works only by coincidence (no validator matches multiple branches), but it's still a correctness issue and a performance waste.
| "Duration", | ||
| "Map", | ||
| ): | ||
| if validator in ("Integer", "Float", "Duration"): |
There was a problem hiding this comment.
Float and Duration are both mapped to <int> which is misleading. A float parameter should show <float> or <number> in syntax, and duration should show <duration> to hint at formats like 5m, 1h, etc.
There was a problem hiding this comment.
Only <int> is documented in searchbnf.conf documentation. I'm quite sure it wouldn't have any impact, if we introduce new keywords like <float> or <duration>.
I guess this issue was created by AI, wasn't it?
What's your opinion on this?
| params_syntax.append(param_syntax) | ||
| else: | ||
| params_syntax.append(f"({param_syntax})?") | ||
|
|
There was a problem hiding this comment.
Magic numbers 120 and 100 for line wrapping. Consider extracting these as named constants (e.g. MAX_SYNTAX_LINE_LENGTH = 120) and adding a comment explaining the rationale.
| or command.get("tags") | ||
| or command.get("examples") | ||
| ): | ||
| logger.warning( |
There was a problem hiding this comment.
The error message on the next line still references syntax as required: "One of the attributes among description, usage, syntax". Since syntax is no longer required (it's auto-generated), this error message should be updated to match.
PR Type
What kind of change does this PR introduce?
Summary
Adds support for custom command syntax auto generation based on command name and parameter definition. If
syntaxparameter is not provided, the syntax will be generated. The value syntax for each parameter is either derived from the validation type or can be defined by thesyntaxattribute of the parameter. It's also possible to omit a parameter from the syntax using thesyntaxGenerationattribute.Changes
Added the attributes
syntaxandsyntaxGenerationto the attribute object, and made thesyntaxattribute on the custom command object optional. Additionally removed the length limit from custom commandssyntaxattribute, because it might be to low for many use-cases.Implemented automatic syntax generation and also formatting with line breaks on a soft limit of 150 characters by line in searchbnf.conf.
This is another update after #1977 and #1979.
User experience
Now the user get's automatically a syntax which includes all command parameters and basic value definition. After this change, the developer must only define parameters, where the automatic generated syntax is not sufficient. It is still possible to override the automatic generation by specifying the
syntaxattribute, so nothing changes for existing projects.Gerneral notes for syntax in searchbnf.conf:
<bool>,<int>or<string>.(high|medium|low), as it shows only the value options, not the parameter.The current implementation is based on my experience, which syntax definitions works best for the given scenarios. I'm still not sure if option lists are a good solution, but they are used in many splunk core implementations, so the generator are also using them.
Checklist
If an item doesn't apply to your changes, leave it unchecked.
Review
Tests
See the testing doc.
Demo/meeting:
Reviewers are encouraged to request meetings or demos if any part of the change is unclear