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

Refactor IngredientController.modules to be a dict #57

Open
cydanil opened this issue May 12, 2020 · 3 comments
Open

Refactor IngredientController.modules to be a dict #57

cydanil opened this issue May 12, 2020 · 3 comments
Labels
enhancement good-first-issue If you want to get involved, these are nice low-barrier issues

Comments

@cydanil
Copy link
Collaborator

cydanil commented May 12, 2020

IngredientController.modules is a list that contains the different UIs ({Description,Ingredient,Instructions,Notes}EditorModules) in the attached to a recipe.

These are now kept in a list, IngredientController.modules, where they are accessed by index.
The correct index for a given module can be found in IngredientController.module_tab_by_name.

This tends to litter the code with a bunch of hardcoded indices, even though the correct way to get a controller is:

idx = i_controller.module_tab_by_name["ingredients"]
ingredients = i_controller.module[idx]

Historically, each EditorModule was kept straight in the class as class attributes. I'm not sure what motivated this change.

However, it would be nicer to simply refactor IngredientController.modules to be a dictionary, to access the modules by their names instead of indices:

ingredients = i_controller.module["ingredients"]
@cydanil cydanil added enhancement good-first-issue If you want to get involved, these are nice low-barrier issues labels May 12, 2020
@maweki
Copy link
Collaborator

maweki commented May 12, 2020

Do we know the attributes beforehand? Then attribute accessors would be a better solution. We get better types from that.

@cydanil
Copy link
Collaborator Author

cydanil commented May 12, 2020

We do, it used to be so before, I don't know what motivated the change.

@maweki
Copy link
Collaborator

maweki commented May 12, 2020

I mean, the we would emulate data classes instead of dicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement good-first-issue If you want to get involved, these are nice low-barrier issues
Projects
None yet
Development

No branches or pull requests

2 participants