Opened 4 months ago

Closed 4 months ago

#32978 closed Cleanup/optimization (wontfix)

Improve an error message on loaddata/dumpdata when PyYAML is not installed.

Reported by: Brad Owned by: Brad
Component: Core (Serialization) Version: 3.2
Severity: Normal Keywords: serializers, yaml, dumpdata, loaddata
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description (last modified by Brad)

While the docs at https://docs.djangoproject.com/en/3.2/topics/serialization/#serialization-formats do mention that PyYAML is required for the yaml serializer, ./manage.py dumpdata|loaddata --format yaml, provides quite a sparse error message, "Unable to serialize database: module yaml not found" (from here: https://github.com/django/django/blob/fbb1984046ae00bdf0b894a6b63294395da1cce8/django/core/management/commands/dumpdata.py#L245) if PyYAML is not installed.

This could be especially confusing for new Django developers who are not aware that yaml is pointing to the PyYAML project on PyPI and is not a part of Django. Since yaml is the only one of the four serializers requiring an add-on third party package, it may be helpful to provide a more verbose error in this case.

For instance, something like. "Serializing to and from YAML requires the PyYAML package. Install PyYAML to use the YAML format."

Change History (12)

comment:1 Changed 4 months ago by Brad

Description: modified (diff)

comment:2 Changed 4 months ago by Brad

Type: UncategorizedCleanup/optimization

comment:3 Changed 4 months ago by Mariusz Felisiak

Component: Core (Management commands)Core (Serialization)
Easy pickings: set
Triage Stage: UnreviewedAccepted

Agreed, we can raise an ImproperlyConfigured error, e.g.

try:
    import yaml
except ImportError as e:
    raise ImproperlyConfigured(
        'Error loading yaml module. Did you install PyYAML?'
    ) from e

comment:4 Changed 4 months ago by Mariusz Felisiak

Summary: Provide more informative error message on loaddata/dumpdata --format yaml when PyYAML not installedImprove an error message on loaddata/dumpdata when PyYAML is not installed.

comment:5 Changed 4 months ago by Bal Krishna Jha

Owner: changed from nobody to Bal Krishna Jha
Status: newassigned

comment:6 Changed 4 months ago by Brad

Interestingly, I found a comment in django/core/serializers/pyyaml.py:

# Requires PyYaml (https://pyyaml.org/), but that's checked for in __init__."

However, I'm not seeing any such check in django/core/serializers/__init__.py. (I expected a HAS_PYYAML or something like that.)

There are also some tests in tests/serializers/test_yaml.py that test specifically for

YAML_IMPORT_ERROR_MESSAGE = r'No module named yaml'

So those will need to be revised.

comment:7 Changed 4 months ago by Brad

Some interesting background:

So it seems that Ticket 12756 was concerned with changing the error from:

Unknown serialization format: yaml

to reporting an ImportError and in that case simply leaving the error message as:

No module named yaml

Last edited 4 months ago by Brad (previous) (diff)

comment:8 Changed 4 months ago by Brad

Having read the background from Ticket 12756, the comment from core/serializers/pyyaml.py about the checking being done in __init__.py makes more sense now. The check that's referring to is here:

https://github.com/django/django/blob/main/django/core/serializers/__init__.py#L70

So again, the import error is caught correctly, but in this ticket I'm proposing adding a friendlier message in the special case of yaml/PyYAML.

comment:9 Changed 4 months ago by Brad

Has patch: set

PR: https://github.com/django/django/pull/14729

Old:

$ ./manage.py dumpdata --format yaml --pks 1 users.User
CommandError: Unable to serialize database: No module named 'yaml'

New:

$ ./manage.py dumpdata --format yaml --pks 1 users.User
CommandError: Unable to serialize database: Error loading yaml module. Did you install PyYAML?

Last edited 4 months ago by Brad (previous) (diff)

comment:10 Changed 4 months ago by Brad

I do realize this is arguably an addition of kludgy code, especially the test_yaml.py changes, in exchange for a small improvement in the error message, so my feelings won't be hurt either way on this one.

comment:11 Changed 4 months ago by Mariusz Felisiak

Owner: changed from Bal Krishna Jha to Brad

comment:12 in reply to:  10 Changed 4 months ago by Mariusz Felisiak

Resolution: wontfix
Status: assignedclosed

Replying to Brad:

I do realize this is arguably an addition of kludgy code, especially the test_yaml.py changes, in exchange for a small improvement in the error message, so my feelings won't be hurt either way on this one.

Thanks for all your efforts! I agree that it's not worth changing.

Note: See TracTickets for help on using tickets.
Back to Top