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

Make multigtfs easily extensible #83

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

Conversation

misli
Copy link
Contributor

@misli misli commented Jun 14, 2018

This change makes multigtfs easily extensible by other Django applications.
All you need is to create model based on multigtfs.models.base.Base
and register the model with decorator multigtfs.models.Feed.register_model.

For example, we use following sexi (Stop External Id) extension in our project:

from jsonfield import JSONField
from multigtfs.models.base import models, Base
from multigtfs.models import Feed, Stop

@Feed.register_model
class StopExternalId(Base):
    """An External Id of the stop.

    This implements stop_external_ids.txt in the GTFS feed.
    """
    stop = models.ForeignKey(Stop, on_delete=models.CASCADE)
    external_type = models.CharField(
        max_length=255, blank=True,
        help_text="Type of the external id")
    external_id = models.CharField(
        max_length=255, blank=True,
        help_text="Value of the external id")
    extra_data = JSONField(default={}, blank=True, null=True)

    def __str__(self):
        return "%s-%s-%s" % (self.stop, self.external_type, self.external_id)

    class Meta:
        app_label = 'sexi'

    _column_map = (
        ('stop_id', 'stop__stop_id'),
        ('type', 'external_type'),
        ('id', 'external_id'),
    )
    _filename = 'stop_external_ids.txt'
    _rel_to_feed = 'stop__feed'
    _sort_order = ('stop__stop_id', 'external_type')
    _unique_fields = ('stop_id', 'type')

@coveralls
Copy link

coveralls commented Jun 14, 2018

Coverage Status

Coverage decreased (-0.8%) to 99.23% when pulling 6f5c788 on bileto:extensible into ca19ee6 on tulsawebdevs:master.

@jwhitlock
Copy link
Member

This is interesting, I still need to think about the approach.

I'm surprised the tests passed, since the processing order of imports and exports was different before, but maybe they reflect an issue with real feeds rather than ones from the tests.

Are you using migrations in your project? How did this code interact with migrations?

@misli
Copy link
Contributor Author

misli commented Jun 19, 2018

I use the same order for import (because it matters). For export the order de-facto doesn't matter, so first I used the same order for both, but than I realized that I need to sort them by the filename to pass the tests. However, I should still write some tests for the registering / unregistering feature. Hopefully I'll get back to it soon.

@misli
Copy link
Contributor Author

misli commented Jun 19, 2018

Regarding the migrations, we use them. As You see in the example, the Meta.app_label needs to be specified for the model to be correctly identified by the migrations tools. (Without the app_label Django assumes that the model belongs to multigtfs.) May be I should add some documentation too.

@jwhitlock
Copy link
Member

I like the idea of ordering exports based on the _filename attribute - that seems like a win by itself, and makes the expected ordering more explicit.

I'm trying to think if there is a more "Django" way to register and order the models. The register methods would require some one-time code to setup the extended models, such as in an AppConfig class. I'm thinking if something like the MIDDLEWARE declaration would work, where a default list is used but could be overridden in settings. I may be thinking too far ahead...

@misli
Copy link
Contributor Author

misli commented Sep 11, 2018

I see registering using decorator a very Python/Django way. It is very similar to registering to Django admin site for example.
In most cases you will register the models in the right order simply because you declare the models in the same order. In some edge cases you may still explicitly call the register method (not using it as decorator) in the right order.
Unlike using some settings attribute, using this decorator registration makes use of third party extension as simple as adding it to INSTALLED_APPS 😉

@misli
Copy link
Contributor Author

misli commented Mar 24, 2020

@daliborpavlovic, do you really mean to update this pull request with the latest changes?
Maybe consider creating another branch for them, based on this one.
@jwhitlock, are You still considering this PR?

@daliborpavlovic
Copy link
Contributor

daliborpavlovic commented Mar 25, 2020

No, sorry, I did not want to update this PR. I was just playing around with our fork and did not know, that this would propagate here. I made reset and force push of the original version, if that is ok.

@jspetrak
Copy link
Contributor

jspetrak commented Apr 2, 2021

We would like to merge this feature. May also provide general solution for #73 without extending multigtfs by non-standard models.

Before merge, test case must be provided. Thank you.

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.

5 participants