Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve format-incremental scripts #996

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mhucka
Copy link
Contributor

@mhucka mhucka commented Mar 4, 2025

This is a rewritten version of check/format-incremental. This was motivated by the desire to add a --help option and do a little bit of cleaning up the code, and then one thing led to another, and, well, here we are. The final changes include:

  • Added a --help option
  • Added a --no-color option
  • Added a --quiet option
  • Make the script more robust in various ways (e.g., declare variables, be more careful in parsing CLI arguments, etc.)
  • Don't hard-code ANSI color code sequences in every echo statement
  • Make the script more DRY
  • Revise the error & info messages to try to be slightly more clear

mhucka added 2 commits March 4, 2025 08:49
This is a rewritten version of `check/format-incremental`. This was
motivated by the desire to add a `--help` option and do a little bit
of cleaning up the code, and then one thing led to another, and, well,
here we are. The final changes include:

- Added a `--help` option
- Added a `--no-color` option
- Added a `--quiet` option
- Make the script more robust in various ways (e.g., declare
  variables, be more careful in parsing CLI arguments, etc.)
- Don't hard-code ANSI color code sequences in every echo statement
- Make the script more DRY
- Revise the error & info messages to try to be slightly more clear
@mhucka mhucka marked this pull request as ready for review March 4, 2025 17:20
@mhucka mhucka requested review from mpharrigan and ncrubin March 4, 2025 18:41
@mhucka mhucka self-assigned this Mar 4, 2025
@mpharrigan
Copy link
Collaborator

Is this shared with the other various repos? Is this the first place the updated script will be committed?

@mpharrigan
Copy link
Collaborator

my personal rule is once you start using a for loop in a bash script, it's probably time to port it to something like python. Is there someone with more bash expertise who can review this?

@mhucka
Copy link
Contributor Author

mhucka commented Mar 7, 2025

Is this shared with the other various repos?

Not yet, but that's my intention. I had to add one for the unitary repo, took the opportunity to rewrite it, and now am going around to add it to the other repos.

Is this the first place the updated script will be committed?

It's in a PR for here and unitary at the momment.

my personal rule is once you start using a for loop in a bash script, it's probably time to port it to something like python. Is there someone with more bash expertise who can review this?

I think what I hear you saying is that it's become too complicated. OK, I worked on simplifying it just now and will push an update.

@pavoljuhas is more expert in Bash than me, so he's the only other one I can think of. I'll add him to the reviewers.

@mhucka
Copy link
Contributor Author

mhucka commented Mar 7, 2025

@mpharrigan The latest commit has a simplified version of the code.

@mhucka mhucka requested a review from pavoljuhas March 7, 2025 22:02
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