-
Notifications
You must be signed in to change notification settings - Fork 639
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
Code formatting, style guide enforcement and static checks #2450
Comments
We had (or have?) pylint on, but it sometimes throws annoying errors which stop a PR merging. i think with something like black, we'd have to have every contributor follow the style, which would mean either a) instructions for everyone on how to apply it easily or b) some sort of CI system which fixes up the formatting on merging. Both seem like they'd add ongoing effort for not much real gain? |
@richardjgowers I just wanted to spark the discussion but I agree the ratio benefits/effort might be small. However, #2409 and #2411 could be easily spotted by static checks. I also have found some indentation errors here and there that will make the code fail when executed. Automatic code formatting (with black or other) is useful only if used with pylint or flake8, so that it removes most of the useless formatting errors. (It might reduce commit diffs if everyone uses it, but this is a very marginal benefit). However, it's really easy to install ( |
I am all in favour of linters and static analyzers. I am considering adding type hints when we would have dropped python 2. I am less keen on formating tools, though: the way I format my code is often deliberate and, sometimes, going against the convention helps with readability or other things. |
I think adding type hints will be a great! I understand that automatic code formatting is not something every developer is willing to accept, but it would be the easiest way to remove most of the linting problems in one go at the beginning. I haven't tried Most of the tools can be configured in |
I'd suggest only applying the CI linting to the I'd be cautious about adopting type hints---maybe if/when they become widely adopted at the base of our dependency tree in NumPy/SciPy, etc. |
I like the idea of linting only the |
There may not be any major drawback, I just mean it could be sensible to wait to see what the rest of the ecosystem decides to do (if anything) about hinting in scientific code. For example, NumPy has an experimental repo for type hinting with arrays: https://github.com/numpy/numpy-stubs See also long-standing issue here: numpy/numpy#7370 Perhaps it may have some influence on what we might want to do. |
Note that SciPy has started accepting PRs that add type hints to source code, so maybe the time for doing this is not really so far away. May want to wait until we officially drop Python 2 support, as noted above. |
+1 on being apprehensive about autoformatters. My main objection is what they do to commit history. It's useful to look at a line of code and see who committed it in which PR, so you can check out any discussion and know who to direct questions to. CI linting the diffs seem like a great idea and would remove some of the PEP8 comments on reviews. |
Just tagging this additional discussion thread here. |
https://github.com/marketplace/pep-8-speaks may be of interest. Edit: instead of adding a check that can fail, it simply comments so contributors and reviewers can decide if they want to follow suggestions or not. |
I installed pep8-speaks for all of MDA — if it’s too annoying we can deactivate it again or only enable it for specific repos. One can also configure it with a config file as described in https://github.com/OrkoHunter/pep8speaks#configuration |
Thanks to @mikemhenry, I realised that it is possible to ignore specific commits when using
For this situation, it is possible to ignore blocks of code or single lines with |
hi there, just adding my two cents – prompted by @RMeli in MDAnalysis/UserGuide#270 (comment) – in my experience code formatters and static analysis tools (black, isort, flake8, mypy) are tremendously valuable. They make the codebase more readable, more maintainable, and lower the bar for entry for new contributors. These tools (in my experience) are the de-facto standard in the industry, and familiarity with them is valuable. Of course, such tools only solve a narrow problem, they don't solve some of the most important feature requests, tech debt or bugs. But they are typically a low hanging fruit with a positive net impact on the health of a codebase and dev velocity. |
Hi everyone, just adding another two cents here -- git has a feature that allows you to ignore certain commits in Sadly it requires a configuration beforehand (you must run I imagine that messing up |
I have been on record as one of those (if not the one) that dislike auto-formatting. I had issues with fixes that were suboptimal readability-wise, portions of the code where the formatting was intentional, and reformatting the existing code base breaking With time, I come to accept that the few places where the formatting is suboptimal are outweighed by the benefit of auto-formatting. We can have guards where the formatting is intentional (like this superb periodic table). If we can ignore some commits for the blame, then all my objections are addressed. There is just the question of how to handle pending pull requests that risk being riddled with merge conflicts. |
I'm still not an amazing fan of
In my opinion, the best approach would be to cut up the process in chunks. That way we can also review the changes in a sane way rather than have a million+ line changes. We work on files that aren't currently in progress, and if we get blocked by a PR that isn't going anywhere then that gives us a good reason to close stale PRs (of which we have many). |
I would however add here (for the @MDAnalysis/coredevs) that I would urge a discussion on priorities here. We have a lot of maintenance work to do, and many PRs that need a push (including all the type hinting ones and the guesser stuff). I'm not saying don't do it, I am however saying let's consider the order in which we do things. |
Is it possible that you add a project-wide autoformatter settings, and then for all new PRs you ask people to a) run the formatter on the files they're changing b) add the respective commit to the |
I don't think that'll work - you'll get conflicts with files where folks are touching the same file. |
But if the settings are project-wide, their autoformatting (should) match unless they touched the same parts of the code? |
EDIT: Sorry folks — I thought this question happened on Egor's PR, not on the dedicated static code formatters issue ...so this is exactly where discussion should happen... oops. So most of what I wrote doesn't apply really. I am just crossing out the embarrassing lines... except the bit that says if anyone wants to push for such a change they better be ready to be responsible for handling it. I think we can cut the discussion pretty short: We can consider autoformatters
So can I ask that if anyone cares enough about the topic of autoformatters, |
I recently started using code formatting (with black), style guide enforcement (with flake8) and static code checks (with mypy). I tried to apply them to MDAnalysis for fun and came across to many warnings.
Describe the solution you'd like
I think it would be great to format MDA with black (or other tools) and then use flake8 and mypy (or other tools) to spot and fix possible problems.
Describe alternatives you've considered
There are other tools that can be considered: pylint, isort, ypaf, ...
Additional context
Many errors come from
MDAnalysis/migration/
.You can have a look at this commit to see the result of automatic code formatting with black.
The text was updated successfully, but these errors were encountered: