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

Enable installing catchpy into an existing Django project #4

Open
wants to merge 43 commits into
base: master
Choose a base branch
from

Conversation

d-flood
Copy link

@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

@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?

Install as a Django app
-----------------------

Add to your `requirements.txt`:

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
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.

@@ -1,3 +1,3 @@
# 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

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)

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
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.

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.

d-flood and others added 20 commits February 27, 2024 05:12
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
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