Skip to content

Conversation

claudep
Copy link
Member

@claudep claudep commented Sep 9, 2017

No description provided.

@adamchainz
Copy link
Member

Could maybe be documented in the django.contrib.postgres docs? inspectdb is typically run on day one of a project, they might not be aware they need to edit INSTALLED_APPS first to get JSONField support.

@claudep
Copy link
Member Author

claudep commented Sep 9, 2017

You're right in that people inspecting a legacy db may not know in advance that a contrib.postgres supported field is present. I'm afraid that putting a note inside django.contrib.postgres docs may not be very helpful in that regard.
What about a hint inside the inspectdb output, so instead of 'This field type is a guess.', people read 'This field type is a guess. You may try adding 'django.contrib.postgresql' in your INSTALLED_APPS setting.? This would however also favor a bit contrib.postgresql over third-party packages.

@adamchainz
Copy link
Member

"This field is a guess. It may be supported by contrib or third party apps." ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually we camel case assertions to match the unittest style. Maybe assertFieldsInModel (considering field_outputs is a list).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be indented inside the if conn.vendor to avoid a redundant check.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you meant inside if conn.connection? We want to update data_types_reverse whatever conn.connection is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if conn.vendor == 'postgresql':
    conn.introspection.data_types_reverse.update({
    ...
    if conn.connection is not None:

(register_type_handlers() also checks conn.vendor == 'postgresql' so that avoids that same check for the non-postgres case)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trailing comma

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test_jsonfield looks good to me (since this is InspectDBTests, _introspection seems redundant).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My inclination is to omit this change as part of this commit. For new users, I think it will raise more questions than answers. I think it's probably better to have some more specific instructions in docs/howto/legacy-databases.txt about it. Considering there's no stable API for it, I'd be surprised if there are any third-party apps that are adding introspection for their fields but if there are some examples, I'd be interested to know about it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chop blank line?

@claudep
Copy link
Member Author

claudep commented Sep 9, 2017

Tentative doc addition.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe wait to add the doc until "several" fields are actually supported?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding RangeFields support is straightforward, the patch is already in good shape.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double the backticks or use

:mod:`django.contrib.postgres`

@timgraham
Copy link
Member

It merits a release note for django.contrib.postgres also:

* ``inspectdb`` can now introspect  ``JSONField`` (``django.contrib.postgres``
  must be in ``INSTALLED_APPS``).

Thanks Adam Johnson and Tim Graham for the reviews.
@claudep claudep merged commit 0cbb6ac into django:master Sep 9, 2017
@claudep claudep deleted the 24928-json branch September 9, 2017 16:28
@claudep
Copy link
Member Author

claudep commented Sep 9, 2017

See #9049 for RangeField.

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