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

more data importing + JSONField #34

Closed
wants to merge 1 commit into from
Closed

more data importing + JSONField #34

wants to merge 1 commit into from

Conversation

MA3STR0
Copy link
Contributor

@MA3STR0 MA3STR0 commented Sep 22, 2011

Hello,
I've added dependency on JSONField and implemented some more importing.

Since I would like to stay within fandjango upstream, I have few questions about code:

  • why not call user.synchronize() during user creation from middleware to avoid code duplication?
  • maybe use 'help_text' field property instead of model class docstring?

@jgorset
Copy link
Owner

jgorset commented Sep 24, 2011

I've added dependency on JSONField and implemented some more importing.

why not call user.synchronize() during user creation from middleware to avoid code duplication?

Great! I don't think we can justify calling user.synchronize() in the middleware, though, since it takes over 15 seconds to complete and we simply can't allow the registration process to take that long. We might consider doing it asynchronously
(i.e. in a different thread), but that would mean the developer never really knows whether the model is ready. Thoughts?

maybe use 'help_text' field property instead of model class docstring?

I think defining help text for each of the fields would be great, but I'm not sure that we should truncate the model's docstring as it is more readily available to developers than help texts are.

@MA3STR0
Copy link
Contributor Author

MA3STR0 commented Nov 3, 2011

Back to Fandjango :)

JsonField looks still crude a bit, e.g. no user-friendly model editing via django forms or admin panel possible.
So, I reverted code to CharFields, and added more of them:

    languages - Comma-separated list of languages
    activities - Comma-separated list of activities
    interests - Comma-separated list of interests
    inspirational_people - Comma-separated list of inspirational people
    sports - Comma-separated list of favorite  sports
    music - Comma-separated list of music
    movies - Comma-separated list of movies
    books - Comma-separated list of books
    television - Comma-separated list of television
    games - Comma-separated list of games
    geolocation - char field for latitude/longitude location
    likes - Multiline string with likes and categories

Importing stuff like facebook "likes" or exact location in one line is not so easy, but solved with list comprehensions.

As you told, calling user.synchronize() is costly, and I think those fields I've added are not really important ones, so I did not populate middleware with import.

Additionally, I've added some properties: age and different sizes of user pictures.

@jgorset
Copy link
Owner

jgorset commented Nov 4, 2011

I really like where you're going with this, but I think there are a few problems with its implementation.

  • User#synchronize is now incompatible with Facebook's official Python SDK. This is not a big deal since we won't be supporting it in the next major version anyway, but if you need these changes to hit before then we'll have to make sure they're compatible.
  • I'm not sure that we should only populate a subset of the fields upon user registration; it's not intuitive that some fields will be missing until the developer synchronizes the user, and there's no easy way to tell whether the user has already been synchronized.

I think we would benefit from converting each of these fields (and some of the existing ones, too) to properties:

class User(models.Model):

    [...]

    @cache(hours=24)
    def _get_likes(self):
        return self.graph.get('%s/likes' % self.facebook_id)

    likes = property(_get_likes)

This approach guarantees that the information is up-to-date, and it's completely transparent to the developer. What do you think?

@MA3STR0
Copy link
Contributor Author

MA3STR0 commented Nov 4, 2011

Cool idea, I did not think about caching possibility, it solves the problem of query cost. The only problem left is filtering on those properties. Its safe for my new fields like movies, books and so on, however could be a problem for others. For example, I'd like to make possible ORM-level filtering by users language. However languages are currently just comma-separated list anyway...
So perhaps I will convert my changes to properties.

Actually it depends on the kind of application, like if its just displaying facebook data then its ok, but if its importing data and then provides forms for editing, properties might lead to problems, as everything has to be copied and safely stored to some other model.

Regarding compatibility issue - sure, I will take a look.

@jgorset
Copy link
Owner

jgorset commented Nov 4, 2011

You make a compelling argument, but at the end of the day I don't think filtering by or editing a user's preferences (e.g. likes, games or music) is a common use case and so it probably isn't worth the complexity it would incur to facilitate it.

As for the compatibility issue, don't fret it unless you really can't wait until the next major version; it should be production-ready within a few weeks.

@jgorset
Copy link
Owner

jgorset commented Nov 8, 2011

I've converted some of the existing fields to cached properties in fa5aff8.

@MA3STR0
Copy link
Contributor Author

MA3STR0 commented Nov 8, 2011

Yes, @cached_property looks like a really elegant solution! Fields like bio, political_views, timezone and so on are just rubbish in the database. Actually, as most of FB fields :) I'm only uncertain about gender and location. This data mostly never change, and for location-aware fb apps it might be important, lets say, for running query selecting all females from the city X, to actually take advantage of Facebook social concept. My Django power is perhaps not enough to think for some workaround in such case. Maybe you have ideas?

Anyway, I will probably work on my fork stuff this Thursday to make it ready for merge in your next major version

@jgorset
Copy link
Owner

jgorset commented Dec 16, 2011

I'm closing this pull request, as the changes it introduces are now features of the next major version (albeit with a slithtly different implementation).

@jgorset jgorset closed this Dec 16, 2011
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.

3 participants