tomage
commented
2 years ago To add: I'd be perfectly happy to either execute, or advise, on executing any of the above.
And a core guiding principle I'd have is: Try and reduce stress+friction when adding these. Doing these changes does more harm than good (at least in the short term) if we block features or bugfixes, or otherwise significantly slow down development, and/or add road-blocks.
It's been a while since I worked on this repo (last commit in early 2014!)...
Now that I find myself in a job writing Go for the most part, I find that I'm craving some python work.
I figured I'd chime in here with some suggestions and thoughts about some general improvements I think could be beneficial to the project. These are all tricks I've learned from working and managing fairly big python projects for several years.
I've got a bunch of suggestions - and I definitely expect some pushback on some of the suggestions. It's not like I can waltz in here, after years of silence, and demand changes :)
I don't think we should do all of these changes at once - but I tried to lay them out in some sensible order in which we could do them:
Code formatter
Run a standardized and configured code formatter on the repo.
I'd strongly suggest
black(written by one of the python core contributors: https://black.readthedocs.io/).blackmostly strives for PEP8, but I always put in some basic configurations. For example: We could (to begin with) allow single-quotes. No need to be that strict. And we could agree on certain line-length. I usually go for PEP8's 79, but black defaults to 88. Django uses 88. Some people like 120 even.I'd also suggest people hook up their editors with black, so it formats on save (making sure that when the edtior does so, it's reading the config file in the base of the repo).
Later on, I'd even suggest we use pre-push git-hooks to make sure nobody is pushing out code breaking the format (I've used https://pre-commit.com/ with good success, but more on that later).
Linter
Setup and configure a linter, and try and tackle most/all of the lints.
I see that few years back, configuration for flake8 was introduced in setup.cfg. There's still something like 500+ lints in the code (NOTE: This goes down to just under 200 after black is run, so :shrug: ).
I'd strongly suggest we ignore many of the lints to begin with, in a configuration file, and then slowly hem in those lints.
I'd also strongly recommend hooking up your editors with the linter (and of course the config file).
Later, we can also run the linter in a pre-push git-hook.
Makefile
I think a simple makefile could ease development a bit. Standard targets, like
make run,make setupetc, could make it easier to both start development, but also just aid development in general.I've even crafted a simple example that we could start out with.
Harden dependencies
I'd strongly recommend we start to pin requirements to at least minor version, but possibly even to patch versions. This reduces random breakage, both for new/current developers, but also in deployment pipelines, and staging/prod environments. It's also way more secure.
I'd also suggest we keep a
requirements.txt.lockfile, which we can update withpip freezewhen we do update requirements, and keeprequirements.txta bit more tidy, and add comments to document why certain libraries are needed. This helps greatly with dependency management.I'd also TBH suggest we use a more nuanced tool, like poetry, but perhaps that's something for later :)
Git-hooks
I'd suggest that we use something like https://pre-commit.com/ to setup some useful git-hooks. Mostly, I've found at pre-push hooks are the least intrusive for development process, but there might be other hooks that people find useful. These can be setup as part of the
make setuptarget, if we add a Makefile.I'd def. consider adding
blackandflake8as pre-push hooks - either right after we introduce these tools, or soon after.Final thoughts
As I said above - feel free to push back on any of the suggestions above. I've done exactly the above in pretty much any project I've been involved in professionally, and it's usually helped catching bugs sooner, and improve readability, maintainability and quality of the code.
I realize that not everyone may be a fan of, say, the style that
blackproduces (and I've been in teams where this was debated), but isn't it better to have a tool autoformat the code, and not spend time arguing about formatting? :)Also: Doing any of the above does add some friction at the start. Developers might be thrown for a bit. Open PR's, or branches branched from current master might have a ton of conflicts after running black for example. But fixing those can be easily done using a simple procedure. I'd be happy to go to all branches after blackifying the code and updating them myself (using rebase, so I'd maintain whatever history you had in those branches).
Anyway.. Been wanting to bring up these suggestions for a long time now, but never quite found the time or energy to do so.