Opened 3 years ago

Closed 3 years ago

#30461 closed New feature (fixed)

contrib.gis.geoip2 does not support pathlib.Path as GEOIP_PATH parameter, only str

Reported by: Nikita Krokosh Owned by: Nikita Krokosh
Component: GIS Version: dev
Severity: Normal Keywords: geoip2
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description (last modified by Nikita Krokosh)

I want to pass Path instance directly as GEOIP_PATH param but currently it only allowes str instances.
As I understand, min Python version for Django 2.2 is 3.5+ so I can freely pass Path instance there.

...
        # Getting the GeoIP data path.
        path = path or GEOIP_SETTINGS['GEOIP_PATH']
        if not path:
            raise GeoIP2Exception('GeoIP path must be provided via parameter or the GEOIP_PATH setting.')
        if not isinstance(path, str):
            raise TypeError('Invalid path type: %s' % type(path).__name__)

        path = Path(path)
...

I've fixed this issue myself.
https://github.com/lsnk/django/tree/ticket_30461_2_2
https://github.com/django/django/pull/11341

Change History (11)

comment:1 Changed 3 years ago by Nikita Krokosh

Description: modified (diff)

comment:2 Changed 3 years ago by Nikita Krokosh

Owner: changed from nobody to Nikita Krokosh

comment:3 Changed 3 years ago by Claude Paroz

Triage Stage: UnreviewedAccepted

Unless this is a regression from 2.1, this won't be backported to 2.2, so I'd suggest targeting the patch to the master branch.

comment:4 in reply to:  3 Changed 3 years ago by Nikita Krokosh

Replying to Claude Paroz:

Unless this is a regression from 2.1, this won't be backported to 2.2, so I'd suggest targeting the patch to the master branch.

I'm new to Django contributing, so can you please tell me, about the branches.
I thought the master branch is for 2.3+ versions, and 2.2 version is inside 2.2.x branch, so if I make PR into master the changes will come only with 2.3+ right?
I thought if I make them into 2.2 they'll hit next 2.2 hotfix and be backmerged into master.

comment:5 Changed 3 years ago by Mariusz Felisiak

The master branch is currently for Django 3.0. This is a new feature so it doesn't qualify for a backport to the Django 2.2 (see supported-versions) and even if patch qualifies for a backport it should be targeted to the master branch.

comment:6 Changed 3 years ago by Mariusz Felisiak

Needs documentation: set
Needs tests: set

comment:7 Changed 3 years ago by Nikita Krokosh

Version: 2.2master

comment:8 Changed 3 years ago by Claude Paroz

Needs documentation: unset
Needs tests: unset

This PR is an alternate patch where I introduced a new to_path utility that we could use in other cases (MEDIA_ROOT, LOCALE_PATHS, ...).

comment:9 Changed 3 years ago by Mariusz Felisiak

Triage Stage: AcceptedReady for checkin

comment:10 Changed 3 years ago by Mariusz Felisiak <felisiak.mariusz@…>

In 88c0b907:

Refs #30461 -- Added django.utils._os.to_path().

comment:11 Changed 3 years ago by Mariusz Felisiak <felisiak.mariusz@…>

Resolution: fixed
Status: assignedclosed

In eed2e740:

Fixed #30461 -- Made GeoIP2 and GEOIP_PATH setting accept pathlib.Path as library path.

Thanks Nikita Krokosh for the initial patch.

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