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

Use class attribute to guide RDN creation #205

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

Conversation

SwampFalc
Copy link

So in #175 it was mentioned that, ideally, the whole process of creating the RDN should be separate from designating primary keys for the database table.

@Natureshadow mentioned they were not keen on such a major rewrite, though.

But looking at the code, I believe there's quite an easy path, namely a simple class attribute, for which I propose rdn_fields, taking a list of field names. The code for build_rdn is trivial to change.

As far as the user is concerned:

  1. They are already expected to add some class attributes. Requiring one more should not be strange or surprising.
  2. Previously, if they did not add a primary_key field, they got an error when saving. Now, if they do not add the class attribute, they get the exact same error at the exact same time.
  3. Creating a list of field names is a common occurrence in Django: for Meta ordering, for the admin site, for a ModelForm, etc.

In short, as far as user experience goes, I do not see this as a major difference.

Is it backwards compatible? Well, not as I wrote it, no. But you can easily turn the if slightly more complicated to achieve that:
if field.db_column and (field.primary_key or field.name in self.rdn_fields):

@Natureshadow
Copy link
Member

@Natureshadow mentioned they were not keen on such a major rewrite, though.

Where did I say that?

In any case, please make this change backwards-compatible.

@SwampFalc
Copy link
Author

I was referring to this comment: #175 (comment)

@Natureshadow
Copy link
Member

I was referring to this comment: #175 (comment)

I can't remember what the reason for this comment was. That said, I will test and review the change in the course of the week.

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