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

Added more FatSecret API functionality #1

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

pmatthen
Copy link

MysteriousTrousers, thanks so much for creating this pod. To use it in my project I had to add the ability to search for recipes and to get specific recipes.

This is my first open-source contribution so hopefully it up to par.

@pwightman
Copy link
Member

Holy cow, this is awesome! I'll take a look through it and give any notes, thanks!

@@ -0,0 +1,13 @@
//
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it looks like there's extras of FSRecipeDirections, FHRecipeIngredients, and FHRecipeImages here that are empty, perhaps you copied them inside the other folder and forgot to delete these ones? Looks like the right ones are in:

/FatSecretKit/FatSecret

whereas these blank ones are just inside /FatSecretKit

@pwightman
Copy link
Member

This is fantastic, thank you! Really happy with this. There's a few things to change to keep inline with the code conventions, and then it'll be good to merge.

  • Remove the extra FSRecipeDirections, etc., files (see my other comment)
  • Change FSRecipeIngredients to FSRecipeIngredient (singular), same with FSRecipeImages -> FSRecipeImage and FSRecipeServings -> FSRecipeServing. FSRecipeDirections can probably stay plural because you don't say "Direction", ever, when referring to recipe directions. So... I'm torn, but let's leave it plural.
  • Is there a difference between the existing FSServing and the FSRecipeServing that you created? Is it possible to just use the existing FSServing class everywhere you're using FSRecipeServing or are there differences? Might help remove some duplication, which would be nice.

Thanks again! Happy to answer any questions, just let me know.

@pmatthen
Copy link
Author

Oh, you're most welcome. Thank you!

Ya, I added the FSRecipeServing, because the info for recipe_serving is different than food_serving (some things extra, some things not there). I'll investigate whether I can pare it down to one class.

Just heading to bed, I'll make the changes tomorrow.

Cheers!

@pwightman
Copy link
Member

Ah, I see. I'd prefer separate classes and some duplication over a subclass or some such, so if they're more than barely different, let's keep them entirely separate like you have it. I'll check that one off in the checklist for now.

Sounds good, no rush. Thanks!

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

Successfully merging this pull request may close these issues.

2 participants