search

soynatan / django-easy-audit

Yet another Django audit log app, hopefully the simplest one.
GNU General Public License v3.0
735 stars 182 forks source link

Modernize package #267

Closed samamorgan closed 7 months ago

samamorgan commented 10 months ago

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

Change summary

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

mschoettle commented 9 months ago

Awesome work! 👍

samamorgan commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

@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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

Yes that makes sense. Here is what I suggest:

I am just a user/contributor though :)

jheld commented 9 months ago

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

samamorgan commented 9 months ago

@jheld @mschoettle Good call, done: #272

samamorgan commented 8 months ago

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 commented 8 months ago

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 commented 7 months ago

@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 commented 7 months ago

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

jheld commented 7 months ago

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

samamorgan commented 7 months ago

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.

image
jheld commented 7 months ago

Unsure yet, just what github is saying

image

samamorgan commented 7 months ago

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

samamorgan commented 7 months ago

@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 commented 7 months ago

@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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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