NOAA-ORR-ERD / PyNUCOS

Python NOAA Unit Converter for OIl Spills
Other
2 stars 8 forks source link

"new" api for getting a conversion without the unit type having to be specified. #2

Closed ChrisBarker-NOAA closed 8 years ago

ChrisBarker-NOAA commented 8 years ago

The current API for the convert function is:

def convert(UnitType=None, FromUnit=None, ToUnit=None, Value=None):

IN teh past, you had to specify teh UnitType -- i.e. "length" of "mass", or...

But Jay has pointed out that most unit names don't conflict, so users should be able to simply do:

convert('m', 'ft', 32)

and not have to know or care what the unit type is or what it's called.

But how to keep teh old API and support this cleanly?

@jay-hennen

ChrisBarker-NOAA commented 8 years ago

Mike Orr wrote in an email:

I found having to specify the unit type slightly annoying too.

The simplest way would be to interpret None in the first argument as meaning "Figure it out yourself."

Why are all the arguments optional? What value is there in calling convert() without the last three arguments?

If it's really unacceptable to delete the first argument, then you can have another function with a different name and deprecate 'convert'.

ChrisBarker-NOAA commented 8 years ago

Mike:

The simplest way would be to interpret None in the first argument as meaning "Figure it out yourself."

yup -- that's what the code is doing now.

Why are all the arguments optional? What value is there in calling convert() without the last three arguments?

because you can't put required arguments after an optional one in Python.

If it's really unacceptable to delete the first argument, then you can have another function with a different name and deprecate 'convert'.

yeah, though I wouldn't delete it, I'd make it the last (optional) argument.

@jay-hennen is thinking that we could be "smart" about it and look at the first argument to determine if it in a unit or a unit-type. that would be a way to keep teh API teh same, but I'm leaning toward a new function name.

Any Suggestions for the name?

jay-hennen commented 8 years ago

Basically, the change would go like so:

unit_conversion.convert( UnitName1, UnitName2, Value ) would be the new function signature. If the user passes in a unit type name, such as "Oil Concentration" as the first argument, then it will basically proceed exactly the way the old convert function did. This behavior will allow compatibility without a second function, and we can officially deprecate this way of using unit_conversion.convert in documentation or with warnings

However, if the 1st argument is not a unit type name, it will then deduce the unit type automatically, check if the destination units is the same type, and then do the conversion.

As a disclaimer, I find this reeks of bad function design, but having two convert functions or having to change the usage of convert everywhere else seems worse.

ChrisBarker-NOAA commented 8 years ago

Jay and I jsut talked this out, and found a hacky (slick?) solution:

pretty much what Jay suggested above, but the signature would be something like

convert(unit_name_1, unit_name_2, value, unit_type=None)

This will be the documented signature -- if the unit names are ambiguous, then the user can specify the unit_type. in the common case, there will be no need.

But as above, if code calls this with the old API, it will still work, and there are no UnitTypes that match unit_names -- so we can check for that.

-CHB

jay-hennen commented 8 years ago

This has been implemented, with tests, in f5dd343 and 916f295