univ-of-utah-marriott-library-apple / python-jamf

`python-jamf` is a library for connecting to a Jamf Server. It maps Jamf Pro records to a Record class. It is the basis for the `jctl` tool to automate patch management & packages and many other items.
MIT License
56 stars 17 forks source link

Consider using Python Black for automatic formatting consistency #62

Closed homebysix closed 2 years ago

homebysix commented 2 years ago

For your consideration, this pull request applies Python Black formatting.

Benefits

In the projects I've worked on, I've found that applying consistent autoformatting (whether Black or something similar like autopep8 or yapf) significantly streamlines contributions. Discussions about code style fade away, allowing contributors to focus on the function.

Also, Black's tendency to split long lines up slightly reduces the risk of merge conflicts if multiple elements of the same list/tuple/dict are edited by two different PRs.

Black sometimes also surfaces syntax bugs before commit, since the formatting won't run if the Python doesn't compile.

Risks

Because this PR touches almost every Python file in the project, it's likely that it will conflict with changes on contributors' branches downstream. Any contributors will need to rebase their changes on the main branch (and possibly resolve conflicts manually) in order to continue. (At the moment, I don't see any non-merged branches here on GitHub, so it's probably a good time for a change like this.)

The most streamlined way to use Black is to configure your local development environment with it, so that every time you save, the formatting is applied. In my VSCode prefs, I have settings to facilitate this:

"python.formatting.provider": "black",
"python.formatting.blackPath": "/opt/homebrew/bin/black",

This optional streamlining creates a small additional setup burden for people who are regularly contributing, but shouldn't prevent those who wish to contribute more casually.

Output

The changes in this PR were produced with black ., resulting in the output below. No changes to actual function were made.

% black .
reformatted jamf/convert.py
reformatted jamf/version.py
reformatted setup.py
reformatted jamf/config.py
reformatted jamf/setconfig.py
reformatted jamf/api.py
reformatted tests/api_mock_test.py
reformatted tests/convert_json_xml_test.py
reformatted tests/test_package.py
reformatted tests/test_config.py
reformatted jamf/admin.py
reformatted jamf/package.py
reformatted tests/test_records.py
reformatted jamf/records.py
All done! ✨ 🍰 ✨
14 files reformatted, 2 files left unchanged.

For more context, see this portion of my 2019 PSU MacAdmins talk, which covers Black specifically.

Thanks for considering!

magnusviri commented 2 years ago

I'm in favor of it. It's not that different. What changed isn't important to me. It's as readable to me after as before. I could probably even get used to the rules and adopt them. Either way, I'm in favor.

The only downside is this creates a bottleneck, a definite before and after. Edit: I thought it would be hard to move between the two but it's actually easy to move forward, just hard to move backwards. I do have local branches that would have to be updated but it would probably be as easy as running black on them.

I don't know of anyone who has made any code changes. I used to be able to click on the number in the "Forks" button at the top of github repos to show me the forks but it doesn't look like that works anymore. Does anyone know what changed? I was going to look at the forks and see if I know them to get their opinion. Is it still possible to view who has forked the project?

magnusviri commented 2 years ago

Oh, one more comment. I ran the checks and it says the CodeQL check is failing. However, when I go to the repo "Actions" page it shows this commit and the checks and it says it passes there. Except I remember seeing these errors before, so I don't think they are new.

I honestly don't know much about CodeQL, I just added it to the checks because it looked like a good idea. Does anyone know anything about it or have comments. The errors are "Sensitive data (password) is logged here." I looked at them before and I don't think they are actually an issue. I should probably look at it again and at least figure out how to avoid the errors but it's not a big priority to me.

magnusviri commented 2 years ago

One question for Elliot. Do you know how to setup BBEdit to automatically apply black? I'm pretty sure BBEdit has the ability. I'm on the BBEdit users list and I could ask them if you don't know.

magnusviri commented 2 years ago

Awwww heck, I'm just going to merge this so that the other changes I'm trying to commit don't block on this. If anyone out there has your own fork you should let us know so we don't do things like this in the future.