Modernize package

[samamorgan]

Modernizes this package to use the ideal tools for building, packaging, testing, and linting/formatting.

Change summary

• Added Poetry as a requirement
• Added Ruff linting/formatting rules to ensure code quality
• Added djLint for Django template linting/formatting
• Added pre-commit as a requirement with git hooks for Ruff and djLint
• Converted tests from unittest to pytest
  • Moved tests out of package directory so they aren't included in distributed builds
• Added a build/publish action that automates package distribution
• Added a contributing guide
• Updated the test action to use new tools and include a code coverage comment for PRs (example at the bottom of this commit)
• Removed Travis configuration since this repo does not use it
• Removed code that resolved issues in older versions of Django and Python that this package does not support
• Refactored model signals to fix multiple complexity concerns

I've added extensive notes explaining the changes in code review comments. If anything is unclear, please ping me here!

[mschoettle]

Awesome work! 👍

[samamorgan]

Thanks!

The test run may fail on creating a coverage comment. The code may have to be merged for that step to have the necessary permissions.

[jheld]

I do have some concerns about the amount of changes taking place in a single PR. Many of these modifications have a reasonable backing for sure, and help to shed some of the baggage of years past, at the very least.

I am a bit hesitant though to agree that all of it may fall under the umbrella of ideal, which implies subjectivity (this is software created by humans, subjectivity is all over the place, grain of salt applied). For instance, I'm not sure that I particularly want poetry in this project at this time. We would benefit from having a pyproject.toml , and precommit, and linting (like ruff) usage, absolutely, to name a few.

So I'm arguing can we split out some things and try to apply them to the project in series? Especially with such a large set of changed files from all of this.

I'm open to being persuaded of course.

[samamorgan]

I do have some concerns about the amount of changes taking place in a single PR. Many of these modifications have a reasonable backing for sure, and help to shed some of the baggage of years past, at the very least.

100% agree. This is a large PR, and well outside what I would usually consider a reasonable number of changes. To be fair, a majority of the changes were automatic, performed by Ruff linting. Since there was no linting standard on this package previously, it's not totally unexpected that the line count change is large given the number of contributors here and different code styles.

The only real logical changes you should see are in the refactoring of the crud flows, and even that was done in the name of reducing complexity according to ruff rules. I did this refactoring before swapping the test suite over to pytest, and the old test suite did pass against these changes, so I have a high degree of confidence that the changes are idempotent.

I am a bit hesitant though to agree that all of it may fall under the umbrella of ideal, which implies subjectivity (this is software created by humans, subjectivity is all over the place, grain of salt applied). For instance, I'm not sure that I particularly want poetry in this project at this time. We would benefit from having a pyproject.toml , and precommit, and linting (like ruff) usage, absolutely, to name a few.

Ideal is certainly not the best word choice. The tools I've added here are generally the most common, active tools that many projects are currently using. I'm quite opinionated about all of this given my experience with these tools, and have benefited greatly from using Poetry especially to automate a lot of the manual things I had to keep in my head before. I particularly like Poetry because it solves a lot of the really strange, hard to resolve dependency issues I've had on larger projects in the past. Plus it comprehensively handles building, venv, etc. It's been pretty fantastic for me in the last ~2 years of use; it works well enough I've converted all of my personal projects and successfully argued for its use in my professional life on a quite large project. Overall the inclusion of that one tool has replaced several others and reduced the cognitive overhead across my entire team.

So I'm arguing can we split out some things and try to apply them to the project in series? Especially with such a large set of changed files from all of this.

I'm all for this, but I'm concerned with the amount of work it will take. I actually considered doing this in stages at first, breaking each bulleted item I've listed above into a separate PR, but the individual PRs don't really illustrate the overall vision I have and I think it's harder to get some of this stuff past reviews without the entire context you see here.

My primary motivation here is to ease developement work for myself and, more importantly, newer contributors to easyaudit. When I started some work on a feature I had in mind, the ergonomics of development were very difficult and I'd really like to make things hopefully simpler for everyone.

[samamorgan]

@jheld I changed the dependency groups to non-optional (optional wasn't necessary, I've been using that flag with incorrect assumptions):

https://python-poetry.org/docs/master/managing-dependencies/

Dependency groups, other than the implicit main group, must only contain dependencies you need in your development process. Installing them is only possible by using Poetry.

I also made CI dispatchable to ease in testing workflow changes. Note that "Add coverage comment" will always fail on a workflow dispatch event, as you can see here: https://github.com/samamorgan/django-easy-audit/actions/runs/7589823375/job/20675243579#step:9:25

The next CI run should pass, with possibly the exception of the "Add coverage comment" step. I've seen this fail on changes from a fork, I believe because of this issue: https://github.com/actions/first-interaction/issues/10#issuecomment-562178406, but it should pass every time after the CI workflow is part of this package's source.

[mschoettle]

The one change I'd argue is helpful to extract into its own PR is the one for #263 since it is unrelated to package organization, CI etc.

[samamorgan]

The one change I'd argue is helpful to extract into its own PR is the one for #263 since it is unrelated to package organization, CI etc.

I certainly could, but that change is required for the test suite to pass currently.

Let me know how or if you guys would like me to split this PR up. I'm happy to put the work in, just want to make some progress before this body of work gets out of my brain space.

[mschoettle]

Yes that makes sense. Here is what I suggest:

• Create a separate PR that only fixes #263 (if it was a separate commit it could be cherry-picked onto a separate branch but I could not see it only looking at the commit titles) [I am happy to do this as well]
• That PR gets merged first
• Then this PR here can be brought up to date and the tests will work
• Usually this PR here could be stacked on top of the other one to include the fix but it already does include it

I am just a user/contributor though :)

[jheld]

concur it would be nice to have the remote addr PR in first.

[samamorgan]

@jheld @mschoettle Good call, done: #272

[samamorgan]

One general comment: It seems that there is an inconsistent use of single and double quotes right now. Cam ruff-format take care of this? Otherwise, we should include the equivalent rule of flake8-quotes. Personally, I am in favour of using single quotes but fine either way.

Double quotes have been the default in black for as long as I've used it, so they seem to have become the de-facto standard in Python. It's such a standard preference that I'd actually have to add additional configuration to use single quotes instead:

https://docs.astral.sh/ruff/settings/#lint_flake8-quotes_inline-quotes

The rule you mentioned is already applied:

https://docs.astral.sh/ruff/rules/bad-quotes-inline-string/

The inconsistency you're seeing is when there's a string that also contains double quotes, like f'a = "b"', which is automatically fixed by this rule:

https://docs.astral.sh/ruff/rules/avoidable-escaped-quote/

[mschoettle]

One general comment: It seems that there is an inconsistent use of single and double quotes right now. Cam ruff-format take care of this? Otherwise, we should include the equivalent rule of flake8-quotes. Personally, I am in favour of using single quotes but fine either way.

Double quotes have been the default in black for as long as I've used it, so they seem to have become the de-facto standard in Python. It's such a standard preference that I'd actually have to add additional configuration to use single quotes instead:

https://docs.astral.sh/ruff/settings/#lint_flake8-quotes_inline-quotes

The rule you mentioned is already applied:

https://docs.astral.sh/ruff/rules/bad-quotes-inline-string/

The inconsistency you're seeing is when there's a string that also contains double quotes, like f'a = "b"', which is automatically fixed by this rule:

https://docs.astral.sh/ruff/rules/avoidable-escaped-quote/

My bad, I missed the double (as in multiple) quotes. Thanks for the references.

[samamorgan]

@mschoettle @jheld Where do we stand on this one? This has been up for review for about 2 months now. I'd like to get a plan at least to get it merged so I don't have to keep fixing merge conflicts. I have other contributions in mind for this package but I can't move on until this one is done.

[mschoettle]

I'm in favour of moving ahead with this. My suggestion would be to merge #276, #277 and then #258 before this one.

[jheld]

sorry @samamorgan , with all of the other PRs, we have a merge conflict.

[samamorgan]

sorry @samamorgan , with all of the other PRs, we have a merge conflict.

@jheld What merge conflict are you seeing? I resolved what I saw this morning, and don't see anything else blocking according to GH.

[jheld]

Unsure yet, just what github is saying

[samamorgan]

I'd recommend changing the merge strategy to squash and merge if you can. Rebasing on every merge seems inappropriate.

[samamorgan]

@jheld I pulled all of my work onto a separate branch, squashed all commits, rebased on master, then force-pushed all of this to my master branch. LMK if that resolves the issue.

[jheld]

@samamorgan much appreciated. I hadn't inspected the merge option at that time so I it didn't occur to me that it might not have been a true conflict. That said, it is quite helpful for these changes to be atomic when possible.

[jheld]

I'm in favour of moving ahead with this. My suggestion would be to merge #276, #277 and then #258 before this one.

I had merged this and just reverted because I remember that #258 was not yet merged. If we don't need to wait for it, let me know and I'll make it right.

[samamorgan]

I had merged this and just reverted because I remember that #258 was not yet merged. If we don't need to wait for it, let me know and I'll make it right.

This PR does actually cover the changes that #258 covers, but if a clear change path is desired, it would technically be better to merge #258 first. @mschoettle That PR has test failures that need to be resolved.

@jheld How do we resolve this now that it's closed and has been reverted? Haven't quite been in this particular situation before.

[jheld]

For keeping things transparent/open, I would have you open another PR with your changes. I could attempt to do it behind the scenes locally once #258 is merged (which is the preference, first), but again, chain of changes tends to flow a little cleaner when going via the MR/PR route.

Apologies for this last hiccup.

[mschoettle]

I’ll fix #258 ASAP to prevent blocking your PR