Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Investigate some simple typing for rates #19

Open
knikolla opened this issue Feb 27, 2025 · 5 comments
Open

Investigate some simple typing for rates #19

knikolla opened this issue Feb 27, 2025 · 5 comments
Assignees

Comments

@knikolla
Copy link
Contributor

knikolla commented Feb 27, 2025

We now have a mix of bools, strings, and numbers.

It is easy to forget to convert the value received from the library to the appropriate type, and that means it's time to consider enforcing some typing for at least string, bools and numbers.

Proposal

  • Adding one new field to each rate called type. type can be of value str, bool, Decimal and corresponds to the Python name of the Datatype.
  • The method get_value_at(name, month) is extended to get_value(name, month, datatype=None)
  • When get_value_at(name, month, datatype=None) is provided, the return string is converted to the requested datatype, provided the datatype matches the type defined in the rate.
  • For each rate that defines type, get_value_at() requires also providing the appropriate datatype class otherwise it spews a warning. For now, we can turn this into a hard error later.
@knikolla
Copy link
Contributor Author

@larsks @naved001 thoughts?

@larsks
Copy link
Member

larsks commented Feb 28, 2025

@knikolla I think adding data types in order to permit data validation is an excellent idea. I wonder about this:

The method get_value_at(name, month) is extended to get_value(name, month, datatype=None)

If the data type is encoded in the data, why do we need a datatype parameter here (or on get_value_at)?

...otherwise it spews a warning. For now, we can turn this into a hard error later.

I would vote for data type violations to be a hard error now.

@knikolla
Copy link
Contributor Author

If the data type is encoded in the data, why do we need a datatype parameter here (or on get_value_at)?

I considered omitting that, but I wanted to force the client to explicitly specify the expected datatype, to work around Python's dynamic typing. Similar to an int received_value = get_value_at(name, invoice_month) call in a statically typed language.

Ultimately, I don't feel strongly about this.

I would vote for data type violations to be a hard error now.

I want to give enough time for the downstream scripts that use nerc-rates to adopt to the new calling signature of the method as described above. This creates an intermediate backwards compatible step in between of the current state and the hard error state.

@knikolla
Copy link
Contributor Author

Another alternative approach is having separate get_decimal, get_string, get_bool methods that return the appropriate datatype if it matches the type, and then deprecating get_value_at.

@QuanMPhm
Copy link
Contributor

QuanMPhm commented Mar 18, 2025

@knikolla I am fine with either implementing a get method for each, or to modify get_value_at's signature. I have a friend who would like to contribute to this issue, and I will tell him to change get_value_at's signature, since I believe that will lead to the simplest diff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants