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

Provide UUIDField option for pk field and a natural key feature for serialization #428

Merged
merged 43 commits into from
Oct 30, 2023

Conversation

mathiasag7
Copy link
Contributor

#427 Feature

Only things haven't written a test case for it. I would have been happy to do it but still learning how to do so.

mathiasag7 and others added 28 commits September 6, 2023 14:24
@Dresdn
Copy link
Contributor

Dresdn commented Oct 15, 2023

Thanks for the PR @mathiasag7. Could you revert back changes that don't specifically apply to the feature? For example newlines, quotes, language changes, etc.? I'll mark them as comments.

pyproject.toml Outdated Show resolved Hide resolved
test_project/settings.py Outdated Show resolved Hide resolved
test_project/settings.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

Merging #428 (eeb86de) into master (964f77d) will increase coverage by 0.57%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #428      +/-   ##
==========================================
+ Coverage   89.87%   90.44%   +0.57%     
==========================================
  Files          23       26       +3     
  Lines         770      816      +46     
  Branches      106      105       -1     
==========================================
+ Hits          692      738      +46     
  Misses         56       56              
  Partials       22       22              
Files Coverage Δ
eav/fields.py 76.47% <100.00%> (+0.96%) ⬆️
eav/logic/managers.py 100.00% <100.00%> (ø)
eav/logic/object_pk.py 100.00% <100.00%> (ø)
eav/migrations/0010_dynamic_pk_type_for_models.py 100.00% <100.00%> (ø)
eav/models.py 96.70% <100.00%> (+0.21%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@Dresdn Dresdn left a comment

Choose a reason for hiding this comment

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

Apologies for the delay on the feedback. I'm recovering from being ill so am catching up on things.

eav/logic/managers.py Outdated Show resolved Hide resolved
eav/models.py Outdated Show resolved Hide resolved
@mathiasag7
Copy link
Contributor Author

I actually thought it might be useful, but the idea came from a project we were working on where we needed to copy data from one database to another from time to time, and to do this we chose django's serialization / deserialization process. To do this, we chose to serialize and deserialize objects. To this we coupled the use of django's feature natural key, which allows unique values to be used instead of pk during serialization. However, if this feature isn't crucial and necessary, we can remove it.

@Dresdn
Copy link
Contributor

Dresdn commented Oct 30, 2023

Thanks for the explanation. Let's keep it but call it out as a new feature. Thoughts?

Edit: Doh! I see you already did that. I'm going to update some docstrings and then get this merged.

Dresdn and others added 4 commits October 30, 2023 08:31
- Added constants for default char field length and field mapping.
- Reorganized imports for better clarity and readability.
- Refactored `get_pk_format` function to leverage a predefined mapping and provide better flexibility.
- Enhanced the documentation for the `get_pk_format` function to explain its purpose and return value.
@Dresdn Dresdn merged commit 3b1be10 into jazzband:master Oct 30, 2023
12 checks passed
@mathiasag7
Copy link
Contributor Author

Hello, I just wanted to inform you that this version of django eav is not yet available to be installed directly with pip in a project.

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