search

BrewCenter / BrewCenterAPI

An open source api for managing homebrewing related data.
32 stars 11 forks source link

Add unit tests for fermentable models. #53

Closed MichaelWashburnJr closed 7 years ago

JayWelborn commented 7 years ago

Can I take a crack at this one, too?

MichaelWashburnJr commented 7 years ago

Yep, let me know if you're still working on this - been out of town for a while

JayWelborn commented 7 years ago

Sure. I'll have a PR ready in the next day or so. I let this slide to the back burner for a few days.

JayWelborn commented 7 years ago

The Fermentable.json() method doesn't function as intended.

Is the goal just to return a string that's valid JSON with the given keys/values? If so I'll re-write the method and send it in the same PR.

JayWelborn commented 7 years ago

I'm also running into errors related to the 'type' attribute. This attribute shadows the name of a built-in function, and it's messing up Django's internals. Is it OK to refactor and change the name to fermentable_type ?

MichaelWashburnJr commented 7 years ago

@JayWelborn I think the fermentable.json() function might not be in use anymore. I believe all the resources use the fermentable serializer to translate it into json now. Try deleting it and see if you get errors. Regarding the type attribute, I'm fine refactoring it to be fermentable_type.

JayWelborn commented 7 years ago

I noticed that you used type on all of your models, so if you aren't getting any errors from it it might not be worth the effort of renaming every reference to every model's type attribute.

MichaelWashburnJr commented 7 years ago

If you're using a linter, you would probably see a redefinition warning, depending on your configuration. It doesn't actually break anything, and is totally allowed in Python. I'm OK with it as-is because in reality it's not even redefining type, since it's within a Class. But I would also understand if someone wanted to change it to something else. So I guess my final say is "take it or leave it"