Skip to content

Enable installing catchpy into an existing Django project#4

Open
d-flood wants to merge 56 commits intomasterfrom
django-package
Open

Enable installing catchpy into an existing Django project#4
d-flood wants to merge 56 commits intomasterfrom
django-package

Conversation

@d-flood
Copy link
Copy Markdown

@d-flood d-flood commented Feb 23, 2024

Change Summary

This PR restructures the Catchpy so that it can continue to be run as a standalone Django app but makes it possible to install Catchpy into an existing Django application.

  • Move apps anno and consumer into the project directory: catchpy. This makes is easier to package Catchpy into a Python wheel.
  • Rename consumer.Profile to consumer.CatchpyProfile to avoid clashes with existing projects.
  • Fixed token creation bug
  • Added requirements file as Dockerfile ARG so that local.txt is default, but docker-compose.yml specifies dev.txt, which enables running tests directly in the dev container.
  • Converted legacy setup.py to pyproject.toml packaged a distribution with hatch
  • Refactor urlpatterns so that the admin URLs are only included when running as a standalone app and the others can be included in an existing project.
  • Handle case when JWT is not prefixed with token details
  • Ensure that CatchpyProfile one to one relationship exists before attempting to access it.
  • Update readme with instructions for adding to existing project and packaging instructions.
  • Update tests

Checklist

  • PR title is descriptive
  • Added/Modified tests for the changes (or explain why not relevant)
  • Manually tested the changes and investigated potential regressions
  • Documented changes (where applicable)

@d-flood d-flood requested review from ColeDCrawford and Yoni-harvard and removed request for ColeDCrawford and Yoni-harvard February 23, 2024 23:14
Copy link
Copy Markdown

@ColeDCrawford ColeDCrawford left a comment

Choose a reason for hiding this comment

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

This is a big PR but it actually looks like this was fairly straightforward, which is awesome. Mostly a lot of moving files around and namespacing packages?

Thanks for the documentation updates - may need to add some more around publishing this to PyPi when we get to that point? The work with custom profiles is also helpful, and good catch with the UTF decoding.

We should check with @nmaekawa on how we want to set up PyPi publishing and versioning for the project - is 2.7.0 the correct bump?

Comment thread README.rst
Install as a Django app
-----------------------

Add to your `requirements.txt`:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is it possible to set these up directly as requirements, so that if you add the CatchPy package it will install these?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good call; in fact, this is already the case but the package dependencies aren't pinned.

Comment thread catchpy/__init__.py Outdated
# important to use single quotes in version string
# for post-commit tagging
__version__ = '2.6.0' # transfer_instructor endpoint
__version__ = '2.7.0' # transfer_instructor endpoint
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maybe update or remove the comment?

Comment on lines -26 to +33
@pytest.mark.usefixtures('js_list')
@pytest.mark.django_db
def test_to_annotatorjs(js_list):
for js in js_list:
catcha = AnnoJS.convert_to_catcha(js)
anno = CRUD.create_anno(catcha, catcha['creator']['name'])
js_back = AnnoJS.convert_from_anno(anno)
assert AnnoJS.are_similar(js, js_back)
# @pytest.mark.usefixtures('js_list')
# @pytest.mark.django_db
# def test_to_annotatorjs(js_list):
# for js in js_list:
# catcha = AnnoJS.convert_to_catcha(js)
# anno = CRUD.create_anno(catcha, catcha['creator']['name'])
# js_back = AnnoJS.convert_from_anno(anno)
# assert AnnoJS.are_similar(js, js_back)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The goal here is to deprecate anything with AnnoJS? Or why is this commented out?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

For me, this test wouldn't pass before or after my changes so I'm not sure what to do and I forgot about it. Thanks for highlighting it.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Got it. We can check with Naomi maybe? I think we should deprecate the AnnoJS stuff as it is unused at this point IIRC, but not sure what the best timing for that is.

ColeDCrawford and others added 16 commits May 28, 2024 18:06
Make the requirements.txt more flexible, so other applications can more easily incorporate Catchpy.
3.12 was failing to install (pointing at old beta).

Could move to a GHA matrix strategy instead of tox.
Replacing pytz with ZoneInfo from Python's standard library as Django 5.0 deprecates pytz.
prime_profile should be parent_profile

Improves error handling of the receiver handler. The old handler was simpler but made some assumptions about the existence and state of the prime_consumer attribute.
3.8 is EOL in October but easy enough to support it until then.
Instead of using tox, use a build argument to control the Python Docker base image to take advantage of GHA parallel / matrix strategy. We end up with multiple containers but they should build faster and be easier to debug than having to manage multiple versions of Python and PATH in one image.
Conditionally import ZoneInfo; use GHA matrix strategy
uv / hatch failing to build in uv MCH with an empty license (`ValueError: Error parsing field `project.license` - Invalid license expression: ''`)
@d-flood d-flood mentioned this pull request Feb 5, 2025
5 tasks
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.

2 participants