Skip to content
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

Modernize package #267

Merged
merged 1 commit into from
Mar 18, 2024
Merged

Modernize package #267

merged 1 commit into from
Mar 18, 2024

Conversation

samamorgan
Copy link
Contributor

@samamorgan samamorgan commented Jan 5, 2024

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This automates version checking, packaging, and deployment to PyPI.

get-package-version checks the current release version against the version in pyproject.toml. If either version can't be parsed according to semver rules, a failure will occur.

If the versions do not match, a new version will be published to PyPI. Note that in order for this to work, a valid PyPI API token with the correct permissions must be added to the repository secrets under the key PYPI_API_TOKEN.

This will only be automatically run against pushes to the master branch. One can also manually trigger this workflow, mostly so that this can be re-run in the event of a failure without having to push a new commit to `master.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworked into ci.yml

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an opinionated replacement of build.yml. The Django matrix was removed since Poetry will automatically resolve the compatible Django version during install.

This will run linters, tests, and add a code coverage comment to the PR.

@@ -78,6 +78,7 @@ celerybeat-schedule
.env

# virtualenv
.venv/
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Poetry creates the environment in .venv.

@@ -87,3 +88,6 @@ ENV/
# Rope project settings
.ropeproject
.idea

# DB
db.sqlite3
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensures the test database is ignored.

@pytest.mark.asyncio
@pytest.mark.django_db(transaction=True)
class TestASGIRequestEvent:
async def test_login(self, async_user, async_client, username, password):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

class TestASGIRequestEvent:
async def test_login(self, async_user, async_client, username, password):
await sync_to_async(async_client.login)(username=username, password=password)
assert await sync_to_async(RequestEvent.objects.count)() == 0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that all of the sync_to_async calls can be removed in favor of using Django's async query methods starting with Django 4.2. This package should drop support for Django <4.2 in April of this year to follow Django's cycle: https://www.djangoproject.com/download/#supported-versions (I'll add an issue for this)

"login_type": LoginEvent.LOGIN,
"username": getattr(user, user.USERNAME_FIELD),
"user_id": getattr(user, "id", None),
"remote_ip": request.META.get(REMOTE_ADDR_HEADER) or "",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was hitting issues with this when testing. This resolved #263

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved into its own PR: #272



@pytest.mark.django_db
class TestAuditAdmin:
Copy link
Contributor Author

@samamorgan samamorgan Jan 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes to this test removed the need for BeautifulSoup, it now uses Django's built-in assertInHTML. Also of note is that the HTML structure changed in Django 4.1 to use summary tags instead of h3.

Additionally, this test wasn't actually asserting a result, so I fixed that. However, I'm of the opinion that we shouldn't even be testing this. This is built-in Django functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I deleted all of the test app migrations and just made a new, fresh one. Should be no issue here, this is test code. The worst possible outcome is contributors will have to delete their local sqlite db, which is no real problem.

@mschoettle
Copy link
Contributor

Awesome work! 👍

@samamorgan
Copy link
Contributor Author

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
Copy link
Collaborator

jheld commented Jan 19, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

samamorgan commented Jan 19, 2024

@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: actions/first-interaction#10 (comment), but it should pass every time after the CI workflow is part of this package's source.

@mschoettle
Copy link
Contributor

mschoettle commented Jan 23, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

Yes that makes sense. Here is what I suggest:

  • Create a separate PR that only fixes Make REMOTE_ADDR optional #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
Copy link
Collaborator

jheld commented Jan 29, 2024

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

@samamorgan
Copy link
Contributor Author

@jheld @mschoettle Good call, done: #272

Copy link
Contributor

@mschoettle mschoettle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went through the changes and added a few comments. Great work! I haven't used poetry myself before so not entirely sure what the benefit of it is. I rally hope that we can get this merged in.

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.

verbose_name = _("CRUD event")
verbose_name_plural = _("CRUD events")
ordering = ["-datetime"]
indexes = [models.Index(fields=["object_id", "content_type"])]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@samamorgan Sorry, I've only noticed this now when I wanted to look at my own PR that fixes this (#258): To me this is also a separate logical change like #272. Is this change required for this PR? If so, maybe we could merge #272, then #258 then this one (if @jheld agrees).


### Security vulnerabilities

If you find a security vulnerability, **DO NOT** open an issue. Email [[email protected]](mailto:[email protected]) instead so as to not expose the vulnerability to the public.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To confirm whether this is the appropriate email address since it seems that only @jheld is maintaining it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's generally reasonable. I'm happy to be in correspondence but yes, there are some items where the nature of it may be more for @soynatan , and I would say so if that was my feeling. But in the end, also up to him :)


### Bugs

When filing an issue, make sure to answer these five questions:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add an issue template for this in a future PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, that would be a good call.

"login_type": LoginEvent.LOGIN,
"username": getattr(user, user.USERNAME_FIELD),
"user_id": getattr(user, "id", None),
"remote_ip": request.META.get(REMOTE_ADDR_HEADER) or "",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved into its own PR: #272

pyproject.toml Outdated
Comment on lines 36 to 37
{version = ">=3.2,<5", python = "<3.10"},
{version = "~5", python = ">=3.10"},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this restrict it to Django 3.2 to 5? Or would a v6 work with this? I am unfamiliar with the poetry syntax. Asking because upper version bounds are problematic: https://iscinumpy.dev/post/bound-version-constraints/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're correct, this isn't quite right. This would restrict Django to >=5.0.0 <6.0.0. Here's the docs:

https://python-poetry.org/docs/dependency-specification/#tilde-requirements

I was trying to match the Python version to the supported Django version table from here:

https://docs.djangoproject.com/en/5.0/faq/install/#what-python-version-can-i-use-with-django

With that in mind, I think this should likely be {version = ">=3.2.9,<6", python = ">=3.10"}. I'd have to test that, but I think it would work correctly. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose to only do >=3.2.9" without an upper bound. This allows users to test/use this package with a later Django version if they want (without having to wait for an update of this package). See the article I linked above.

Copy link
Contributor Author

@samamorgan samamorgan Feb 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, I'll make that change. Figuring out just the right specifiers for the wide range of Django and Python version compatibility was a bit difficult, so I appreciate you calling this out.

"D203", # Docstrings on class definitions not preceded by a blank line
"D213", # Multi-line docstring summary should start at the second line
"D407", # Dashed underline after doc section (not compatible with google style)
"DJ008", # Model does not define `__str__` method
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be done in a follow-up PR

"RUF012", # Mutable default values in class attributes
]
line-length = 92
select = [ # https://docs.astral.sh/ruff/rules
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity: Are there specific ones that you chose not to include?

]

operations = [
migrations.AlterField(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are all the AlterField operations caused by?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many of these fields allowed null=True, blank=True, which is bad. That means there are two possible values with the same meaning. This rule explains it well:

https://docs.astral.sh/ruff/rules/django-nullable-model-string-field/

I changed all of them to blank=True, default="", which will not propagate None as a value, but still allow the field to be blank.

@samamorgan
Copy link
Contributor Author

samamorgan commented Feb 29, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

samamorgan commented Mar 12, 2024

@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
Copy link
Contributor

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

@jheld
Copy link
Collaborator

jheld commented Mar 18, 2024

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

@samamorgan
Copy link
Contributor Author

samamorgan commented Mar 18, 2024

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
Copy link
Collaborator

jheld commented Mar 18, 2024

Unsure yet, just what github is saying

image

@samamorgan
Copy link
Contributor Author

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

@samamorgan
Copy link
Contributor Author

@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
Copy link
Collaborator

jheld commented Mar 18, 2024

@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 jheld merged commit dc43262 into soynatan:master Mar 18, 2024
6 checks passed
@jheld jheld mentioned this pull request Mar 18, 2024
@jheld
Copy link
Collaborator

jheld commented Mar 18, 2024

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
Copy link
Contributor Author

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
Copy link
Collaborator

jheld commented Mar 18, 2024

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants