Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#33839 closed New feature (wontfix)

Support pathlib.Path in loaders' get_template().

Reported by: simon klemenc Owned by: simon klemenc
Component: Template system Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

Since pathlib is the modern approach to file-paths we should probably support pathlib.Path in all template loaders

affected PRs:
https://github.com/jrief/django-admin-sortable2/issues/320
https://github.com/django/django/pull/15835

Change History (6)

comment:1 by Mariusz Felisiak, 2 years ago

Owner: changed from nobody to simon klemenc
Status: newassigned
Summary: support pathlib in template loadingSupport pathlib.Path in loaders' get_template().
Triage Stage: UnreviewedAccepted
Version: 4.0dev

Seems reasonable. Thanks.

comment:2 by Mariusz Felisiak, 2 years ago

Has patch: set
Needs documentation: set
Needs tests: set

comment:3 by David Smith, 2 years ago

I am not sure we need to support pathlib.Path?

In the first example there is:

os.path.join('adminsortable2', app_label, opts.model_name, 'change_list.html'),

os.path.join returns a str. If it wanted to be remove the use of os I think string concatenation using f-string or %-format would also work? e.g.

f'adminsortable2/{app_label}/{opts.model_name}/change_list.html'

Assuming, my above statement is correct. Is there a concrete example where using pathlib.Path over string concatenation is preferable?

I'm also thinking about Jinja2 given the original PR targeted that backend. It's docs say that the template name should be a str or Template. I'm not sure it's Django's responsibility to enhance that API to do conversion of Paths?
https://jinja.palletsprojects.com/en/3.1.x/api/#jinja2.Environment.get_template.

comment:4 by Carlton Gibson, 2 years ago

TBH I think I'm -1 on this as well.

I see no benefit in loosening the accepted type on the template_name argument, from str to Union[str, Path] as in the draft PR.
It's a looser API, that doesn't match the underlying expectations, as David points out.

I'd suggest casting to str as a last step if want to use pathlib for the manipulations.

comment:5 by Mariusz Felisiak, 2 years ago

Has patch: unset
Needs documentation: unset
Needs tests: unset
Resolution: wontfix
Status: assignedclosed
Triage Stage: AcceptedUnreviewed

Agreed with David and Carlton.

Simon, please follow the triaging guidelines with regards to wontfix tickets and take this to DevelopersMailingList if you don't agree.

comment:6 by Carlton Gibson, 2 years ago

I'll just note, the API contract here isn't to paths but to template names. Really it's an implementation detail of the loaders that those names map to the file system. A DB-based loader (say) would be free to adopt a totally different naming scheme, such that getting passed pathlib.Path instances would be unexpected, shall we say.

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