refactor result and some PEP8 fixes

#9 Merged
  1. Sławek Ehlert

related to issue #64. There's a one nasty side effect (which fails the tests) of making DEFAULT_MAX a module level var at selectable/forms/

Comments (5)

  1. Mark Lavin repo owner

    Finally getting around to some review and I have some initial comments.

    I'm not sure have max_limit on the lookup class is a good idea or works as intended. This same validation is done by the form and so the max_limit will only work if it is lower than the default. I'm not keen on having multiple places to do the same thing.

    _get_options doesn't seem particularly useful. It's marked as internal and I can't really think of a case where this would need to be changed. Maybe I'm just being short-sighted here.

  2. Sławek Ehlert author

    Ok. Point taken.

    I agree on the max_limit issue. Generally I think that there should be some limit on every lookup (because notice that the rest of the code won't work if there's no pagination) and initially that was the motivation for the max_limit.

    As for _get_options it shouldn't be internal maybe. It could be useful in situation when we use a custom form rather than the default BaseLookupForm.

    Cheers from Poland and good night ;)

    1. Mark Lavin repo owner

      I'm ok with saying that the SELECTABLE_MAX_LIMIT is required in the sense that it can no longer be None. Then we could document the path of overriding the form if the user wanted/needed to change this limit for an individual lookup.

      I'm going to make some tweaks on this branch and start working on the necessary JS changes. If my changes seem way off base then please let me know. If you have additional changes then update the pull request and I'll pull those into the branch as well. I'll either update this pull request or create another to review the branch before this change hits default so that we are all on the same page with the changes.