Details
- Reviewers
bcooksley - Maniphest Tasks
- T3689: Add abi compliance checker to CI
- Commits
- R857:bb51725a79f7: Add ABI Dump Creation utility.
Diff Detail
- Repository
- R857 CI System Tooling
- Lint
Lint Skipped - Unit
Unit Tests Skipped
This looks okay for the most part, mostly just coding style fixes.
Only big question is the one on splitting out the code into a module in helperslib/ as I made in one comment. Thoughts about that?
helpers/create-abi-bump.py | ||
---|---|---|
23 | Coding style issue: elsewhere i've used descriptive variable names which indicate what it's contents should be. | |
26 | Coding style: please see gather-jobs.py for how braces for maps/arrays are usually formatted. | |
31 | Could you add some comments, indicating what this function expects to receive and what it provides as a result? | |
36 | Same as above. | |
48 | Coding style: same issue as for line 25. | |
50 | Can we have some descriptive variables here as well please, along with some comments on what we're trying to achieve? | |
60 | Coding style: brackets. | |
69 | Should this class, and perhaps the above function be split off into a module into helperslib/? | |
80 | Please use os.path.join() to ensure all edgecases around paths being merged are handled. | |
82 | Coding style: as we've moved on to the next unit of work there should be a newline to separate the writing of the CMakeLists.txt file and the invocation of CMake. | |
87 | Coding style / readability: the formatted version of the regular expression should be stored in a variable then that should be run through re.match | |
92 | Coding style: descriptive variables please | |
97 | As above: should this function perhaps be separated out for ease of readability and future reuse? | |
107 | Coding style: braces | |
119 | Can we get a comment describing why we're ignoring system prefixes and anything relating to KDE Frameworks? | |
139 | Could we get some comments describing what we're trying to achieve and how we're going to do that please? | |
144 | Coding style: can the variable be made more descriptive please? | |
156 | Coding style: please split the generation of the filename and revision identifier out to their own lines for readability. |
helpers/create-abi-bump.py | ||
---|---|---|
23 | well match is mostly used for the return value of an re.match. ret is a very clear name -as it is the return variable. | |
31 | no module level do not make sense, as those inner functions use local variables targets/variables. Additional this function depends on the part at line 56. | |
69 | I don't think so as this is clearly not a clean way to parse cmake - as it is only a dirty hackish way to parse. A better start for a good parser would be https://salsa.debian.org/qt-kde-team/pkg-kde-jenkins/blob/master/hooks/prepare/cmake_update_deps but only if we plan to parse more than this simple two keywords. | |
231 | Don't know if we should implement a propper way to save for data in the metadata file in package. |
helpers/create-abi-bump.py | ||
---|---|---|
55 | I know it's mostly "nitpicking" but it would be best to assing meaningful names to args[0] and [1] where it makes sense: if len(_args) == 2: variable, value = _args variables[variable] = value and so on. | |
80 | Since pathlib is used below, I would argue to use Path instead, it's much nicer and better than the os.path functions (at the cost of some overhead). | |
94 | Can we compile this just once at the start of the module? I don't think it's going to be much of a problem. | |
131 | What if this fails for some reason? Or is this guaranteed never to? |