DFHack
Add astyle config and check style in CI
Adds a standard astyle config and a CI check for code style. The first 4 points of the config here will enforce current style requirements as specified in Contributing.rst. The rest of it is some things to consider standardizing that have worked well for us on the CDDA team. Anyone who's regularly developing here should check out how it fits their preferences.
Closes #1535
To do:
- [ ] Decide on any additions to standard project style for the config
- [ ] If any changes are made that current code doesn't meet add a file blacklist so CI doesn't always fail until standard style is met by all files
- [ ] Update any relevant documentation
The rigid CDDA "Artistic" style enforcement is utter garbage. It's "artistic" as in ugly, contrary to common conventions, in complete disregard for what's practical and useful, and undocumented apart from some feeble examples that sort of hints at what some of the rules are, enforcing usage of a specific command line tool (the installation instructions are out of date, relying on a magic population of the tool into a list where it isn't available, so command line usage to uglify the code is required, at least for Windoze).
I share some of your concerns about astyle's ease of use. I think #1535 is still relevant (it was inspired by feedback on a PR whose style differed significantly from the rest of the DFHack codebase) so we could discuss alternatives there. I've kept this open to serve as a reminder to take a more thorough pass on this specifically tool+config (sorry I haven't yet) even if we don't ultimately settle on it.
If some kind of style enforcement is implemented, it would have to:
- Focus on the important bits, not an obsessively detailed OCD level (that hits you with full force in the face when you modify anything as the existing code doesn't abide by it).
- Be compatible with the general style used in DFHack, rather than something completely different (which should also be compatible with how code editors generally format things, rather than the complete opposite of it).
- Be properly documented (every rules explained in a manner understandable by the average reader). If you're not prepared to explain it, don't mandate it!
- Provide extensive and useful feedback to inform the victim of exactly where and why it rejected the code with references to the corresponding rule(s).
- Have a set of rules the average contributor could learn with relative ease so they're not required to run some obscure black box "fixer" thing that might have done something (or not) to contribute anything.
I just threw this together to show how styling would easily fit into the existing CI after we were talking about it on the IRC. Normally I use clang-format in my own stuff. You get more versatility out of it than astyle. If lundell cant read cata style code or operate the tools just use a different config with clang-format.
You miss the point that if you mandate something you force everyone to abide by it, so I wouldn't have any option to format things in a sensible way if the CDDA rules were enforced.
Saying that if the tool installation instructions are incorrect you can then investigate how to use some other tool to do the enforcement you didn't want in the first place and then research what the rules enforced are and how to code them to fit that other tool just doesn't make any realistic sense.
It's not that I can't decode the code that results from what CDDA rules mandates, it's that I can't consistently write it in that eye hurting manner and the compiler automatically reformat it into a more readable (and thus rejected) format anyway, leading to a rejection whenever a line break is introduced at any time in the process. And, of course, that most of the rules are hidden so I don't even know what they are.
You miss my point. This PR was demonstrating concept. I'm not invested in what formatting standard you use for your project or which tool it uses. I just found when I was writing my other PR last year that automated style checks and a formatting config for my editor would have caught things I had to fix sooner.
I'm definitely on board with something for style formatting. Thanks for putting this up! We have since switched away from Travis, so it would need some modifications in any case, but I'm happy to handle that.
Actually coming up with formatter configs that match our existing code style will be somewhat complicated. I think it's important to minimize the burden for existing contributors (I think Patrik is getting at this too), both in terms of running the formatter and producing code that matches our style guidelines in the first place. I would generally like to have something that's easy to run locally on all of our target platforms, so if astyle has portable binaries, that could be a good option. I know we'll also get contributors who don't install formatters locally, so some way to correct formatting (through CI?) instead of simply rejecting it might also be useful (but that depends less on the exact formatter we use). In any case, it's something that will need to be documented and accessible (so probably on the Contributing page).
From my personal experience, I've found that running code through formatters locally tends to lower the effort required on my end in the long run, since I don't have to worry quite as much about fixing stylistic issues manually.
