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

UrlForMixin now offers a url property (resolves #193) #200

Merged
merged 3 commits into from Nov 22, 2018
Merged

Conversation

jace
Copy link
Member

@jace jace commented Nov 16, 2018

No description provided.

@jace jace requested a review from iambibhas November 16, 2018 10:19
@jace
Copy link
Member Author

jace commented Nov 16, 2018

Several models have a url column. The column will override the property, but we'll have to careful about unintended mixups and should consider renaming those columns.

https://github.com/search?q=org%3Ahasgeek+%22url+%3D+db.Column%22&type=Code

@jace
Copy link
Member Author

jace commented Nov 21, 2018

I'm a little concerned about forcing a url property to be available on all models just because UrlForMixin is a base class of BaseMixin. We need this to be more intent-based:

RoleMixin should look for a dictionary named proxy_properties or __proxy_properties__ mapping names to functions. These will be made available in the proxy as properties.

Syntax 1 (read-only properties):

class MyModel(BaseMixin, db.Model):
    proxy_properties = {
        'url': lambda self: self.url_for(),
        }

Syntax 2 (read/write, but verbose):

class MyModel(BaseMixin, db.Model):
    proxy_properties = {
        'url': {'get': lambda self: self.url_for()},
        }

Neither syntax offers a way to declare what roles these properties are available under. Presumably, that will be left to __roles__.

On the other hand, if url turns out to be widely used property, this syntax is unnecessary verbiage in all models. We could take yet another approach: a new mixin dedicated to adding URL properties:

class UrlPropertyMixin(object):
    @with_roles(read={'all'})
    @property
    def url(self):
        return self.url_for()

Models that need url can simply inherit from this class. Other models are not affected.

@iambibhas
Copy link
Contributor

I like the UrlPropertyMixin idea but can we call it absolute_url instead of url? And let it be available to all models? I'm getting the idea from Django where each model has a get_absolute_url() method that's used internally in Django whenever a url is needed for a model. It can be overridden as well. https://docs.djangoproject.com/en/2.1/ref/models/instances/#get-absolute-url

This lets us get out of this url name confusion.

@jace
Copy link
Member Author

jace commented Nov 22, 2018

If we call it absolute_url, it doesn't need to be in a separate mixin.

@jace
Copy link
Member Author

jace commented Nov 22, 2018

Renamed. Good to merge, @iambibhas?

@jace jace merged commit bf25849 into master Nov 22, 2018
@jace jace deleted the url-property branch November 22, 2018 09:02
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.

None yet

2 participants