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

Support custom fields without corresponding model attributes #2569

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

shouichi
Copy link
Contributor

The current implementation assumes fields have corresponding model
attributes. But sometimes, we just want to display a button or a link
that is not backed by an attribute.

This change simply relaxes the assumption and allows custom fields that
are not tied to model attributes.

See also #1468.

The current implementation assumes fields have corresponding model
attributes. But sometimes, we just want to display a button or a link
that is not backed by an attribute.

This change simply relaxes the assumption and allows custom fields that
are not tied to model attributes.
@nickcharlton
Copy link
Member

Thanks for opening this!

How does this compare to #2658?

@shouichi
Copy link
Contributor Author

shouichi commented Sep 24, 2024

This PR is simpler than #2658 (only one line change in lib and other changes are just for tests). It just relaxes the assumption, no feature was added. Each application can implement its virtual attributes and administrate doesn't need to know anything.

@goosys
Copy link
Contributor

goosys commented Sep 25, 2024

I tried creating a similar NonAttributeField in the #2658 branch, and it worked perfectly.
Unless there’s a specific intention to raise an exception when accessing an attribute that doesn’t exist in the model, I believe this approach is sufficient.

@nickcharlton
Copy link
Member

Thanks, that's helpful.

I think I conceptually prefer this, as the code change is much smaller compared to the other. But I need to think about it!

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.

3 participants