addons/namingng.py: Reinstate dict-with-regexps, cosmetic overhaul. #5824
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In 7c168fa (Feature/ros naming check, Jan 9, 2019) a feature was implemented where regular expressions can be given in a dict, i.e. `{"[a-z]+":[arg0,arg1]}` instead of an array `["[a-z]+"]`. There is only the namingng.py code, an example `ROS_naming.json` and some GitHub discussion to go at when trying to understand the feature. The feature was not readily understandable by looking only at the code. It tested the first argument using `if ~arg0` (i.e. using bit-wise negation), which only evaluates as true when `arg0 == -1`, while the example JSON and discussion indicate that `arg0` is actually boolean. The code seemed to indicate some form where `arg1` is not used, and `arg0` would contain a string, but it was not obvious how this should work. This feature seems to be intended as follows: When specifying a list, a positive match of each item is required. When specifying a dict, the dict keys are regular expressions and their values are `[bool,string]` with bool indicating what match result is to be reported as an error, and string a message that is appended to the reported error. This approach has various benefits: the config can specify an explanatory message, and it is now possible to more simply forbid and/or require certain patterns. Recent changes, especially 98b9244 (addons/namingng.py: Add tests for include guards, config file validation, Jan 2 2024) completely broke this feature, while keeping the code in. This patch provides a rewrite of the feature, adds sanity checks to the configuration, and provides unit tests for both the configuration of the feature and the feature itself. The dict feature does not work for `RE_FILE` (yet). The example config file `ROS_naming.json` is modified to prevent an error triggered by the dict value of `RE_FILE`.
…ngth. This patch is (/should be) purely cosmetic, and aims to improve maintainability by reducing indentation, function length and redundant code/comments. The following was done: - put every check in a separate function with the same signature - use args.debugprint instead of function kwarg debugprint - eliminate almost-duplicate code - simplify/remove redundant code, e.g. `if a is None or a != 'Public'` - remove stale or all-too-obvious comments - remove unused arguments - reorder code flow to reduce indentation (`if not a: continue`) - fix recent, minor bug that prevented standalone mode to work properly The results: - the number of "tabs" is reduced by 30% - the maximum indentation depth is now 5, coming from 8 - the longest function is now ~75 lines, coming from ~150 lines - 9 functions were added - a net total of 6 lines of code were removed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
namingng.pysomewhat supported specifying a dict instead of a list for regular expressions, until the feature was broken by a patch of mine recently. This PR contains a patch rewriting the feature and expanding relevant unit tests.To improve maintainability, a second patch is added that refactors the code for better readability and structure.