hotzenklotz / pybeerxml

A simple BeerXML parser for Python
MIT License
19 stars 16 forks source link

Get the original values for ibu/fg/og/abv/color values from the recipe? #17

Closed scheb closed 4 years ago

scheb commented 4 years ago

First off, thanks for the great library! Helps me a lot, as I'm currently parsing BeerXML recipes for data analysis :)

I've recignized that the values for ibu/fg/og/abv/color are calculated from the recipe ingredients, so there's no way to retrieve the values originally provided in the recipe XML. Do you see any chance to make these values somehow available? E.g. via an og_beerxml (or similar) attribute?

hotzenklotz commented 4 years ago

Thanks for your interest. That is a interesting idea. I guess none of my original recipes had those attributes pre-computed and hence I calculated all of this.

I am bit hard pressed for time right now, but I am happy to accept pull requests with community changes. So feel free to send me a pull request and will include it.

scheb commented 4 years ago

Sure, I can send you a PR. How would you do it? I see some options:

1) Keep the calculated properties as they are and add extra fields under a different name for the original XML values 2) Rename the calculated properties to something making clear that it's a calculated value (e.g. abv_calculated) and let the properties return the original XML values (this would be a BC break, would require to be releases as 2.0) 3) Implement some kind of fallback logic into the calculated fields, like: return the original XML value if it's there, but if not return a calculated value (this would also be a BC break)

Let me know what you're thinking.

hotzenklotz commented 4 years ago

I am in favor of 3.. Why do you think this is not backward compatible? The property name stays the same. Perhaps, one could argue that the value might change in some cases for some people.

I am not against breaking compatability / having a 2.0 release if that is required.

scheb commented 4 years ago

The property name stays the same. Perhaps, one could argue that the value might change in some cases for some people.

Yes, that's what I mean. The behaviour of the library changes. You'd potentially get a whole different value with that implementation.

hotzenklotz commented 4 years ago

The property name stays the same. Perhaps, one could argue that the value might change in some cases for some people.

Yes, that's what I mean. The behaviour of the library changes. You'd potentially get a whole different value with that implementation.

I'd still prefer that approach, if you don't mind. That should guarantee most users to receive a value. (In any case, we could add a debug log message, letting users know that the access property was calculated to let them know.)

scheb commented 4 years ago

I'm totally fine with that. I'd add some extra methods to explicitly retrieve the calculated values, so in case someone wants these, there's a way to get them.

Regarding the debug message, I'm not sure how to do that properly. I'm relatively new to the Python world. Would be great if you could point me to an example.

hotzenklotz commented 4 years ago

I'm totally fine with that. I'd add some extra methods to explicitly retrieve the calculated values, so in case someone wants these, there's a way to get them.

Sounds good to me.

Regarding the debug message, I'm not sure how to do that properly. I'm relatively new to the Python world. Would be great if you could point me to an example.

Use the Python logging module: https://docs.python.org/3/library/logging.html

I was thinking about something like this:

import logging

logger = logging.getLogger(__name__)

@propery 
def get_ibu():
   ...
   logger.debug("Note: The value for IBU has been calculated from your hop bill using Tinseth's formula") 

This will log the hint to stdout (similar to print()) but can be disabled globally using the right enviroment variables etc

scheb commented 4 years ago

Ok, I'll have a look at this and send you a PR.

hotzenklotz commented 4 years ago

Solved by #20