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

Framework to allow EXTRA_FORM_FIELDS #434

Merged
merged 1 commit into from
Oct 20, 2012

Conversation

seanvoss
Copy link
Contributor

Before this get's lost.

Here is a pull request based on what I think was decided in the Forum.

Register a form field within the context of the settings.py

@stephenmcd
Copy link
Owner

Awesome Sean, thanks for kicking this off. Just a few comments:

  • The ID for the field type shouldn't be dynamically calculated. These get stored in the DB and so if another field type was added to mezzanine.forms, everyone's code would break, so we should make it so that the developer has to explicitly define the ID.
  • The code for reading the setting and updating fields.NAMES, fields.CLASSES etc should go directly into mezzanine.forms.fields - mezzanine.boot is generally for Django-specific things that are project-wide, and have special requirements in terms of Django loading up, specifically models and admin classes. This new code is only specific to mezzanine.forms.fields, and if it lives directly in there, we're 100% guaranteed that anything else that imports from there will have the correct values.
  • When we set up new settings, they should go into one of the defaults.py modules (in this case mezzanine.forms.defaults). This means that the settings automatically get documented (see http://mezzanine.jupo.org/docs/configuration.html#default-settings) and it keeps things consistant as well. It'll also mean that by using mezzanine.conf.settings.EXTRA_FORM_FIELDS, we'll always have a value for it, so we don't need to use getattr when checking that the setting is actually defined.
  • Extending the last point, we probably don't need this setting commented out in the project's settings.py - the ones that are in there are generally settings people would almost always want to configure, whereas this new setting doesn't seem like it would be a commonly used one.

@seanvoss
Copy link
Contributor Author

So, more like that then? You now have to define the ID as well.

Let me know if you want me to make the code more defensive.

@seanvoss
Copy link
Contributor Author

Defined as.

FORMS_EXTRA_FIELDS = (
    (
        1337,
        "captcha.fields.CaptchaField",
        "captcha"
    ),
)

@stephenmcd
Copy link
Owner

Yeah that's great Sean, thanks for the update.

@stephenmcd stephenmcd merged commit 2d166f6 into stephenmcd:master Oct 20, 2012
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