search

hotzenklotz / pybeerxml

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

Add misc object and separate it from hops #5

Closed kjoller closed 6 years ago

kjoller commented 6 years ago

I had trouble using the current version as MISC objects does not have the same attribute as HOP objects, and it was messy checking which was which.

This changes behavior (if you expect miscs to be in hops), but I believe it to be more correct.

hotzenklotz commented 6 years ago

@kjoller Thanks for the PR. LGTM. Do you mind adding a valid recipe to the tests to check the new misc category. Thanks.

kjoller commented 6 years ago

I have added one of my recipes from http://beercalc.org. That site does not allow you to input a boil time for misc ingredients, so I added it by hand. I am not sure how and what to test in a misc. It seems as simple as a yeast.

hotzenklotz commented 6 years ago

@kjoller I added a test for your recipe and misc objects. I am unsure how I can add the commit to this PR. Perhaps you can enable rights for me to add the commit directly on your fork: https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/

BTW, I noticed two more things:

  1. Your miscobjects have a type attribute altough your recipe does not include those. Are type attributes part of the standard?
  2. The amount_is_weight is a boolean. However the parser will interpret it as a string "TRUE". That is fine for me right now. Any thoughts?
kjoller commented 6 years ago

The Allow edits from maintainers is already ticked. If you are having troubles, it may be because it is the master branch. I do not have Github-foo enough to fix it :-(

  1. The TYPE is part of the standard, and is actually required. I guess I should send a bug report to the Beercalc team. I added all attributes from the standard as None, actually without looking at my own XML at all.

  2. I would like to look at it, but I do not think I will have the time the next couple of days, but would like to fix it at some point. Could we let the pull request hang for a couple of days?

kjoller commented 6 years ago

I have implemented boolean in the same way as the fermentable.add_after_boil. And I think that we should go forward with this for the time.

However, the method makes even the string "FALSE" show up as boolean 'True', so it should probably be adjusted. The examples on beerxml.com use 'TRUE' and 'FALSE' strings as booleans, so it should probably work. This is a separate issue, which I will happily start work on next.

kjoller commented 6 years ago

Also, I have invited you to be a collaborator on my fork, hoping it will give you write-access to it.