Opened 17 months ago

Closed 17 months ago

#34676 closed Cleanup/optimization (fixed)

Normalise MeasureBase unit not found exceptions to use AttributeError

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

Description (last modified by Andrew Northall)

I am making use of the Distance class in contrib.gis.measure in my project and it is annoying to have to constantly catch Exception when checking if a particular unit exists. It would be better to have a specific exception for this case.

​https://github.com/django/django/pull/17006

Change History (9)

comment:1 by Andrew Northall, 17 months ago

Cc: Andrew Northall added
Description: modified (diff)
Owner: changed from nobody to Andrew Northall

comment:2 by Mariusz Felisiak, 17 months ago

I don't think that separate exception class is justified, I'd change to the ValueError.

in reply to:  2 ; comment:3 by Andrew Northall, 17 months ago

Replying to Mariusz Felisiak:

I don't think that separate exception class is justified, I'd change to the ValueError.

Thanks - if you'd rather not have a class, would it perhaps be better to normalise all unit not found exceptions to AttributeError?

default_units() already returns AttributeError if the units are not found (before I changed it in my PR), so would it be better just to make unit_attname() also return AttributeError to match (and hence retain backwards compatibility with people who may depend on default_units() returning AttributeError?

Or would you leave default_units() as AttributeError and have unit_attname() return ValueError?

Version 1, edited 17 months ago by Andrew Northall (previous) (next) (diff)

in reply to:  3 ; comment:4 by Mariusz Felisiak, 17 months ago

Replying to Andrew Northall:

Replying to Mariusz Felisiak:

I don't think that separate exception class is justified, I'd change to the ValueError.

Thanks - if you'd rather not have a class, would it perhaps be better to normalise all unit not found exceptions to AttributeError?

default_units() already returns AttributeError if the units are not found (before I changed it in my PR), so would it be better just to make unit_attname() also return AttributeError to match (and hence retain backwards compatibility with people who may depend on default_units() returning AttributeError)?

Or would you leave default_units() as AttributeError and have unit_attname() return ValueError?

We should unify both exceptions. I'd use AttributeError as it will be backward compatible.

comment:5 by Andrew Northall, 17 months ago

Summary: Add DistanceUnitDoesNotExist exception to contrib.gis.measure → Normalise MeasureBase unit not found exceptions to use AttributeError

in reply to:  4 comment:6 by Andrew Northall, 17 months ago

PR updated

comment:7 by Mariusz Felisiak, 17 months ago

Triage Stage: Unreviewed → Accepted

comment:8 by Mariusz Felisiak, 17 months ago

Triage Stage: Accepted → Ready for checkin

comment:9 by Mariusz Felisiak <felisiak.mariusz@…>, 17 months ago

Resolution: → fixed
Status: assigned → closed

In 38cde27:

Fixed #34676 -- Normalized Distance()/Area() exceptions for nonexistent units.

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