tulsawebdevs / django-multi-gtfs

Django app to import and export General Transit Feed Specification (GTFS)
http://tulsawebdevs.org/
Apache License 2.0
50 stars 32 forks source link

Make multigtfs easily extensible #83

Open misli opened 6 years ago

misli commented 6 years ago

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 commented 6 years ago

Coverage Status

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

jwhitlock commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 5 years ago

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 :wink:

misli commented 4 years ago

@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 commented 4 years ago

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 commented 3 years ago

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.