-
Notifications
You must be signed in to change notification settings - Fork 498
Add pre commit hooks to catch basic formatting issues #1530
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
Conversation
Add pre-commit configuration containing some basic formatting checks.
Trims trailing whitespace and fixes incorrect (non LF) line endings.
| ### Supported protocols and digital modes: | ||
| * [__AX.25__](https://www.sigidwiki.com/wiki/PACKET) using 2-FSK or AFSK for modules: | ||
| * [__AX.25__](https://www.sigidwiki.com/wiki/PACKET) using 2-FSK or AFSK for modules: | ||
| SX127x, RFM9x, SX126x, RF69, SX1231, CC1101, RFM2x, Si443x, LR11x0 and SX128x | ||
| * [__RTTY__](https://www.sigidwiki.com/wiki/RTTY) using 2-FSK or AFSK for modules: | ||
| * [__RTTY__](https://www.sigidwiki.com/wiki/RTTY) using 2-FSK or AFSK for modules: | ||
| SX127x, RFM9x, SX126x, RF69, SX1231, CC1101, nRF24L01, RFM2x, Si443x, LR11x0 and SX128x | ||
| * [__Morse Code__](https://www.sigidwiki.com/wiki/Morse_Code_(CW)) using 2-FSK or AFSK for modules: | ||
| * [__Morse Code__](https://www.sigidwiki.com/wiki/Morse_Code_(CW)) using 2-FSK or AFSK for modules: | ||
| SX127x, RFM9x, SX126x, RF69, SX1231, CC1101, nRF24L01, RFM2x, Si443x, LR11x0 and SX128x | ||
| * [__SSTV__](https://www.sigidwiki.com/wiki/SSTV) using 2-FSK or AFSK for modules: | ||
| * [__SSTV__](https://www.sigidwiki.com/wiki/SSTV) using 2-FSK or AFSK for modules: | ||
| SX127x, RFM9x, SX126x, RF69, SX1231, CC1101, RFM2x and Si443x | ||
| * [__Hellschreiber__](https://www.sigidwiki.com/wiki/Hellschreiber) using 2-FSK or AFSK for modules: | ||
| * [__Hellschreiber__](https://www.sigidwiki.com/wiki/Hellschreiber) using 2-FSK or AFSK for modules: | ||
| SX127x, RFM9x, SX126x, RF69, SX1231, CC1101, nRF24L01, RFM2x, Si443x, LR11x0 and SX128x | ||
| * [__APRS__](https://www.sigidwiki.com/wiki/APRS) using AFSK for modules: | ||
| * [__APRS__](https://www.sigidwiki.com/wiki/APRS) using AFSK for modules: | ||
| SX127x, RFM9x, SX126x, RF69, SX1231, CC1101, nRF24L01, RFM2x, Si443x and SX128x | ||
| * [__POCSAG__](https://www.sigidwiki.com/wiki/POCSAG) using 2-FSK for modules: | ||
| * [__POCSAG__](https://www.sigidwiki.com/wiki/POCSAG) using 2-FSK for modules: | ||
| SX127x, RFM9x, RF69, SX1231, CC1101, nRF24L01, RFM2x and Si443x | ||
| * [__LoRaWAN__](https://lora-alliance.org/) using LoRa and FSK for modules: | ||
| * [__LoRaWAN__](https://lora-alliance.org/) using LoRa and FSK for modules: | ||
| SX127x, RFM9x, SX126x, LR11x0 and SX128x | ||
| * Supports Class A and C (and Multicast over C) | ||
| * Pre-certified for Class A on dynamic channelplans (EU868-style) | ||
| * See the [wiki](https://github.com/jgromes/RadioLib/wiki/LoRaWAN) and [notes](https://github.com/jgromes/RadioLib/blob/master/examples/LoRaWAN/LoRaWAN_Starter/notes.md) for more information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing whitespaces are necessary in Markdown to prevent formatting issues.
|
Overall, I'm not entirely opposed to adding some automated linter, however, I'm unsure about this implementation. For example, the Markdown docs require trailing whitespace as a newline. This is probably an easy fix just by skipping this check for .md files, but it nicely illustrates the broader point that sometimes there can be non-trivial consequences of these automated tools. In the past I was experimenting with uncrustify (#676), but I ran into a problem with GitHub actions runners that were raising strange errors while running it, so eventually I removed the action. It might be time to revisit this since it's been more than 2 years ago. Is there some advantage to using pre-commit as opposed to uncrustify (apart from uncrustify's 3000+ lines of config file)? |
|
@jgromes In terms of catching formatting errors, there isn't much difference regarding what tool is used to catch/fix them (i.e., pre-commit formatting hooks vs uncrustify). The main benefit I see from using pre-commit over GH actions for minor issues is that IMO it's easier to setup/run pre-commit locally (only requires python and a single pip package). Thus, from a workflow perspective, minor issues such as formatting/spelling will get caught and fixed locally by each contributor (since pre-commit runs automatically on each commit) and never reaches the review stage. In theory, it should also be possible to expose a way to run GH actions locally. However, ensuring that local machine of different platforms has the requisite packages/tools will typically require some sort of docker file. |
|
@Dazza0 I agree that it's not realistic to expect people that want to contribute to this repository to run GH actions locally. Though that is not the only option, for a lot of actions here I just call a script in Though in any case, this will need to be part of the CI because we have a lot of one-off contributors, fixing a very specific thing. I don't think we can expect these to install our specific linting tool, whatever that tool turns out to be. So I'd rather go back to uncrustify, have it in a separate script and run it on all PRs from CI. |
|
@jgromes Sounds good to me |
Description
Noticed that a lot of files have trailing whitespaces which may get automatically trimmed by the code editor, thus polluting the Git diff when making changes.
This PR adds pre-commit to this project along with some hooks to catch basic formatting issues (e.g., line endings and trailing whitespace):
CONTRIBUTING.mdto mention pre-commitCould also consider adding a spell check and uncrustify to the list of hooks.