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

Clean RecCard #132

Merged
merged 3 commits into from
Jun 26, 2020
Merged

Clean RecCard #132

merged 3 commits into from
Jun 26, 2020

Conversation

cydanil
Copy link
Collaborator

@cydanil cydanil commented Jun 24, 2020

This is a follow-up of #130.

Here, we're cleaning the RecCard class by adding annotations, removing unreachable code, and modernising properties decorators.

rec_gui, formely rg, is never None, a fact that was highlighted by the fact that the conditional import doesn't even work, thus removed.

Comment on lines 66 to 69
self.rec_display: Optional[RecCardDisplay] = None
self.conf: List = []
self.new: bool = True if recipe is None else False
self.current_rec: 'RowProxy' = recipe if recipe else self.rec_gui.rd.new_rec()
Copy link
Collaborator

Choose a reason for hiding this comment

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

First of all, good work! Also on the set_ and get_ translations.

Second: Are all these attributes to be "written" by other methods? Some of these look like they should be hidden via double-underscores, others are functional or read-only and could be properties. Do you have any deeper insight?

If we can slim the interface down even further, it would be great.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was not keen to tackle that, but doing so revealed that RecCard.conf is a leftover from a previous architecture, and can be removed!

cydanil added 2 commits June 24, 2020 23:47
A more in-depth refactoring is needed to remove it
@cydanil
Copy link
Collaborator Author

cydanil commented Jun 24, 2020

I spoke too soon: RecCard.conf cannot be removed at the moment without introducing ugliness elsewhere, at least in GourmetRecipeManager, line 443:

def quit (self):
for c in self.conf:
c.save_properties()
for r in list(self.rc.values()):
for c in r.conf:
c.save_properties()

The rest is okay, though

@cydanil cydanil mentioned this pull request Jun 25, 2020
@cydanil
Copy link
Collaborator Author

cydanil commented Jun 26, 2020

If there are no further comments, I'll merge in a few hours, thanks!

@cydanil cydanil merged commit 3f7a922 into kirienko:master Jun 26, 2020
@cydanil cydanil deleted the feat/reccard_rg branch June 26, 2020 14:58
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