Run black on all source files
Closed this issue · 7 comments
Hi!
Preamble
I'm currently preparing a number of issues/PRs for Aequilibrae. With my company we're running a (not yet public) fork of Aequilibrae for which I've recently become the maintainer. You may already have heard of us through our ex-colleague @barisdemirdelen. Now, I would like to look at the possibility to bring (some of) our changes into Aequilibrae proper, which would it easier for us to maintain our fork and gives Aequilibrae some newer/enhanced features/capabilities. Our changes include:
- Support for multiple projects
- Some threading changes (or perhaps customizability/configurability of some threading aspects)
- Some (test) performance changes
- Getting rid of some warnings/deprecations
/end-of-preamble
I've noticed that you are using black (judging from the .pre-commit-config.yaml) for code formatting. This is great! However, not all code seems to be blackened yet. I would like to keep the PRs I'm preparing clean, but right now precommit is blackening every file I'm touching so there's also a bunch of changes that are just code formatting. I would like to suggest to create a single commit/PR that blackens all of the codebase, so any further PRs are not muddied by code format changes. The PR would consist of the following
- update .pre-commit-config.yaml to point to a specific black version
- add a pyproject.toml so that black can easily be run from commandline with the specified line length
- update .gitignore (black automatically ignores everything in .gitignore to ensure it doesn't format files it doesnt need to)
- run black
Now, this commit/PR would muddy git blame, since it would appear that the commiter changed all files. Black proposes a solution for that in the form of an ignore-revision (see https://black.readthedocs.io/en/stable/guides/introducing_black_to_your_project.html). There would be a second commit that adds the first commit's hash to a .git-blame-ignore-revs file. This file can then be passed to git blame (and is also automatically passed to Githubs blame server) so that those changed lines would still be attributed to the correct commit.
What do you think of this?
Hi @elfjes, first off welcome to aequilibrae and thanks for contributing! I remember @barisdemirdelen brought some important fixes and it is great to hear you are still using aequilibrae.
Regarding this issue and the linked PR, Pedro and I have talked about needing to do this at some point, but we never got around to it. I wasn't aware of the git blame solution, that's really helpful!
Now, I would like to look at the possibility to bring (some of) our changes into Aequilibrae proper
That sounds promising, looking forward to it :)
That is great, @elfjes . I have made two comments on that PR (where one is really for @janzill to check).
I am a little apprehensive, however, about the changes you have made to threading, as the weird threading we have is basically targeted at supporting the QGIS plugin, but let's discuss those when we get to it.
we're running a (not yet public) fork
@elfjes , this note worries me a little bit as well. Is there a plan to have a public (and competing) fork of AequilibraE at some point in the future?
Substantial changes are coming to AequilibraE (including transit assignment), so it will be pretty hard to maintain competing forks out there.
That is great, @elfjes . I have made two comments on that PR (where one is really for @janzill to check).
I am a little apprehensive, however, about the changes you have made to threading, as the weird threading we have is basically targeted at supporting the QGIS plugin, but let's discuss those when we get to it.
I get it, which is why I'm also not sure yet what would be the best solution. We're not using aequilibrae on qgis however, so we simplified some things. I guess a maintainable solution for aequilibrae would be to provide some option to plugin a custom threading solution (sounds complex) or perhaps provide an option to disable threading altogether (since it doesn't really add anything outside a qt context). But I haven't gotten around to that PR yet, so yeah, we can discuss it later :)
As to the fork part. Yes it is my plan that all of it becomes public as part of a bigger effort to open up part of our software. For our aequilibrae fork I see two paths:
- we can bring all our required changes to aequilibrae so that we won't need to run a fork (would have my preference, but we need to see if it's feasible) or
- we will run a separate (open) package with (a few) cherry picks, that follows aequilibrae as closely as possible. It is not my desire to go separate ways, merely we need to use aequilibrae in a way to which it's currently not 100% compatible
That's a fair assessment, @elfjes . My preference is also for option #1, and I am happy to work with you guys on a solution that allows for that, as that would also mean that there are more people working on the core software that is available to everyone.
The threading portion will probably be a challenge, but I am happy to look into a solution that contains minimum overhead in terms of code complexity. Maybe duplicate some of the code in the plugin? Maybe eliminate the threading capability for some quick processes (e.g. gravity models).
Merged. Thanks, @elfjes !