Repository navigation
Split storage from models.py into models_future - #1476
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe pull request moves S3 storage and manual-upload models into ChangesStorage models
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Refactor Suggested reviewers:
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 34.92% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 63 functions across 6 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly and concisely describes the main change: moving storage models from models.py into models_future. |
| Description check | |
The description includes a summary, change list, related issue, detailed implementation notes, and test instructions. It does not include the checklist, deployment notes, or a reported test result, bu… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
- Commit to this branch
- Create a new PR
🛠️ Fix failing CI checks 💡
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
- Commit to this branch
- Create a new PR
- Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
kra
marked this pull request as draft
October 6, 2026 00:14
✅ Deploy Preview for antenna-preview canceled.
|
✅ Deploy Preview for antenna-ssec canceled.
|
Collaborator
Author
|
|
kra
marked this pull request as ready for review
October 6, 2026 22:24
mihow
added this pull request to stack #1489
October 8, 2026 16:38
kra
force-pushed
the
kra/model-split-tests-storage
branch
from
October 8, 2026 22:14
ccac90d to
a3aec65
Compare
kra
force-pushed
the
kra/model-split-tests-storage
branch
from
October 8, 2026 22:58
a3aec65 to
7e171c2
Compare
mihow
approved these changes
Oct 9, 2026
Collaborator
|
@kra great start! If this passed the CI tests then I think its safe to merge |
…from main.models to models_future.storage. Use strings to references classes in (django.db.models.)ForeignKey calls to avoid circuar references, since main.models needs to import this module. main.models re-exports models moved to models_future under their same models.* names for compatibilty with existing migrations. I'd rather update the migrations, but that's a much broader change and I didn't want to think about what implications I might be missing. I didn't update any references to ami.main.models.S3StorageSource to use ami.main.models_future.storage. I feel like that's the right thing to do, but the name "models_future" makes that questionable? We probably should just do that to make our embarrasment more obvious? Maybe when we've moved enough to name main.models to main.models_legacy, at which point we'll need to deal with existing migrations? Tested with `docker compose -f docker-compose.ci.yml run --rm django python manage.py makemigrations --check --dry-run`
kra
force-pushed
the
kra/model-split-tests-storage
branch
from
October 9, 2026 21:02
11aae91 to
55f218f
Compare
mihow
added a commit
that referenced
this pull request
Oct 10, 2026
…was squash-merged #1461 landed on main as one squash commit, so its files reached main with no history link to this branch's earlier merges of the #1461 branch. Each add/add conflict was resolved by keeping this branch's side, which is #1461's final version of the file plus #1469's changes: main's copy of every conflicting file except ami/ml/results/tests.py equals the #1461 head this branch had merged. That test file takes main's version, which adds the test for #1461's last commit (the read-only results admin). Main's other changes (#1476, #1420, #1495) merged cleanly. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QT59KePky4u4nbCsTAggtc
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Move classes S3StorageSource, SourceImageUpload, and related methods from main.models to models_future.storage.
List of Changes
This is based off of the branch in #1470.
Moves code from ami.main.models to ami.main.models_future, with minor edits, and adds tests to models_future.
Related Issues
Relates to #260.
Detailed Description
Move classes S3StorageSource, SourceImageUpload, and related methods from main.models to models_future.storage.
Use strings to references classes in (django.db.models.)ForeignKey calls to avoid circuar references, since main.models needs to import this module.
main.models re-exports models moved to models_future under their same models.* names for compatibilty with existing migrations. I'd rather update the migrations, but that's a much broader change and I didn't want to think about what implications I might be missing.
I didn't update any references to ami.main.models.S3StorageSource to use ami.main.models_future.storage. I feel like that's the right thing to do, but the name "models_future" makes that questionable? We probably should just do that to make our embarrasment more obvious? Maybe when we've moved enough to name main.models to main.models_legacy, at which point we'll need to deal with existing migrations?
How to Test the Changes
docker compose -f docker-compose.ci.yml run --rm django python manage.py makemigrations --check --dry-runSummary by CodeRabbit