Skip to content

Initial commit of the common trick header dev branch - #67

Draft
jman025 wants to merge 2 commits into
mainfrom
40_common_trick_header_dev
Draft

Initial commit of the common trick header dev branch#67
jman025 wants to merge 2 commits into
mainfrom
40_common_trick_header_dev

Conversation

@jman025

@jman025 jman025 commented Jul 23, 2026

Copy link
Copy Markdown

Refs #40

@ninotarantino

Copy link
Copy Markdown
Collaborator

This is looking pretty good, just a few more edge cases we'll want to fix.

Some general script comments:

  • Instead of specifying a file with -f, can we have the filename be a positional argument? bin/cml_common_trick_header.py <args> filename.
  • Along those lines, let's have the script handle multiple files at once. My goal is to essentially run git ls-files '*.hh' '*.cc' and pipe the output to the script with the -c flag as part of our CI pipeline.
  • Minor nit, but since the script modifies files I think it should be named something with a verb, like apply_cml_trick_header_format.py or something.
  • Right now the script warns on not having PYTHON_MODULE in the Trick header for source files but that directive only applies to headers. We'll want it to check that the file it's processing is either a .h or .hh file for the python module check.
  • Looks like the script doesn't recognize the ICG: directive right now. From the Trick documentation it can be [ICG: (No|Nocomment)].

The purpose section is placed within a second set of parenthesis but it should just stay at one. So I'm seeing the script convert

PURPOSE: (some purpose)

to

PURPOSE:
(
 (some purpose)  <-- should be no parenthesis here
)

I'm also seeing the script choke on Trick headers that use a cascading '*', like the following:

/*
 * PURPOSE: (test)
 *
 * LIBRARY DEPENDENCY: 
 *  (
 *   (../src/test.cc)
 *  )
 * /

In cases like that, it actually removes everything except the purpose field in the formatted output. This may end up being a nightmare to fix so just let me know if it's feasible to handle this case or not. Realistically, headers formatted this way will already fail our formatting check so the script does its main job, and just from looking around randomly I don't believe this is a common pattern in the repo anyways.

For long lines, let's add one more indent when wrapping text just for visual clarity. So for the following header:

/*
   PURPOSE: (Test)
  
   LIBRARY_DEPENDENCY:
    (
     (../src/test.cc)
    )
  
   PROGRAMMERS:
    (
     ((Nino Tarantino) (CACI) (July 2026) (This is a very long comment that should get wrapped at 80 characters.))
    )
 */

The programmers section ends up like

PROGRAMMERS:
    (
     ((Nino Tarantino) (CACI) (July 2026) (This is a very long comment that
     should get wrapped at 80 characters.))
    )

But I think it would look clearer if the line break was indented to visually separate a continued line from a new entry. Especially if there ends up being multiple entries:

PROGRAMMERS:
    (
     ((Nino Tarantino) (CACI) (July 2026) (This is a very long comment that
       should get wrapped at 80 characters.))  <-- I think one space of indent would look fine, too
     ((Another contributor) (Company) (July 2026) (Another))
    )

@ninotarantino ninotarantino left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should've left that previous comment as part of the official GitHub review, whoops.

- last list of command line args will be the list of *.hh or *.cc files to be changed
- file renamed to apply_cml_common_trick_header.py
- script only warns for no PYTHON_MODULE or no cml/cml.* modules present for *.hh files
- added ICG: directive
- removed nested parenthesis for PURPOSE and ICG directories
- script can handle the " /* ***... */ " C/C++ style comments for Trick headers
- added extra indentation for lines that exceed the character limit

Refs #40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants