CARTAvis / carta-backend

Source code repository for the backend component of CARTA, a new visualization tool designed for the ALMA, the VLA and the SKA pathfinders.
https://cartavis.github.io/
GNU General Public License v3.0
22 stars 11 forks source link

Normalize the style of guard name from headers #1326

Closed markccchiang closed 12 months ago

markccchiang commented 1 year ago

Description

Checklist

confluence commented 1 year ago

There are a few issues with this approach:

I think that the cleanest way to implement this (which is also portable, and sidesteps the sed compatibility issue) is to extend the style.py Python script (as suggested in the issue). I have committed what I think is a working implementation to a branch -- please have a look and confirm that it works (it should give the same result as your bash implementation). In addition to detecting incorrect guards and missing guards, it separately reports when the closing endif comment doesn't match the guard name.

To perform a check, run ./scripts/style.py headerguards check, and to perform a fix, run ./scripts/style.py headerguards check. This additional check type is also included in all, so it should be picked up automatically by the CI. In my branch I have just committed the script change, not any modified files.

If you're happy with that approach, please feel free to merge that branch into your PR (or to make a new PR; whatever is simpler).

markccchiang commented 1 year ago

There are a few issues with this approach:

  • The script should be callable from the main directory, not from the scripts subdirectory.
  • The sed implementation test should not rely on a platform check, because the user could have GNU sed installed on MacOS. This check should also be done in one place and not repeated.
  • This script only does replacements, and doesn't have an option just to check for incorrect guards.
  • It doesn't automatically fix missing guards.
  • We need this check to be performed automatically on PRs, just like the other style checks (and the automated check should only report, not fix).

I think that the cleanest way to implement this (which is also portable, and sidesteps the sed compatibility issue) is to extend the style.py Python script (as suggested in the issue). I have committed what I think is a working implementation to a branch -- please have a look and confirm that it works (it should give the same result as your bash implementation). In addition to detecting incorrect guards and missing guards, it separately reports when the closing endif comment doesn't match the guard name.

To perform a check, run ./scripts/style.py headerguards check, and to perform a fix, run ./scripts/style.py headerguards check. This additional check type is also included in all, so it should be picked up automatically by the CI. In my branch I have just committed the script change, not any modified files.

If you're happy with that approach, please feel free to merge that branch into your PR (or to make a new PR; whatever is simpler).

@confluence Thank you for providing the new edition of ./scripts/style.py! I confirm it works well and is a better way to implement this guard name style check. So I removed the original bash script ./scripts/check_guard_name_style.sh.

github-actions[bot] commented 12 months ago

Code Coverage

Package Line Rate Health
src.Cache 63%
src.DataStream 44%
src.FileList 67%
src.Frame 36%
src.HttpServer 42%
src.ImageData 28%
src.ImageFitter 83%
src.ImageGenerators 43%
src.ImageStats 72%
src.Logger 34%
src.Main 52%
src.Region 17%
src.Session 4%
src.Table 52%
src.ThreadingManager 67%
src.Timer 85%
src.Util 30%
Summary 32% (6023 / 18675)