Skip to content

Conversation

@brunobord
Copy link
Contributor

@brunobord brunobord commented Aug 29, 2018

Review

  • Tests
  • Docs/comments
  • CHANGELOG.rst Updated

Copy link
Contributor

@alexdashkov alexdashkov left a comment

Choose a reason for hiding this comment

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

LGTM, just a few comments

return field_class(**self.get_field_kwargs())

if self.field_is_dict:
params = self.field.get('parameters', None)
Copy link
Contributor

Choose a reason for hiding this comment

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

get('parameters', None) == get('parameters')

for key, classpath in extra.items():
module_name, class_name = classpath.rsplit('.', 1)
module = importlib.import_module(module_name)
field_builder_class = getattr(module, class_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to move import part to the function in the utils (we will use it to get token in the future)

import six


def import_object(object_path):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

great, I love this

@brunobord brunobord force-pushed the form-filler-extra-fields branch from 8a723f5 to 8e7b312 Compare September 3, 2018 08:36
brunobord and others added 2 commits September 12, 2018 14:46
* Added documentation,
* Added a settings-based loading mechanism to extend the `FormFieldFactory`
* Added validation mechanism for the dynamically loaded fields
@alexdashkov alexdashkov force-pushed the form-filler-extra-fields branch from 4ae3af9 to 24d65b9 Compare September 12, 2018 12:46
@alexdashkov alexdashkov requested a review from syldb September 12, 2018 12:46
Copy link

@syldb syldb left a comment

Choose a reason for hiding this comment

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

LGTM. This context was such a pain! We should remember to be careful when updating DRF

@brunobord brunobord changed the title WIP: Form filler extra fields Form filler extra fields Sep 13, 2018
@alexdashkov alexdashkov merged commit 50c6438 into master Sep 18, 2018
@alexdashkov alexdashkov deleted the form-filler-extra-fields branch September 18, 2018 08:28
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.

4 participants