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

Bug fixes #134

Merged
merged 5 commits into from
Jul 2, 2024
Merged

Bug fixes #134

merged 5 commits into from
Jul 2, 2024

Conversation

Josh-Voyles
Copy link
Owner

This PR factors the backend, and solves a lot of bugs.

self.setWindowTitle("Home Choice Pro")
self.guide = self.open_guide()
self.ui.guideLabel.setText(self.guide)
self.pmi_warned = False
self.not_pmi_warned = True
Copy link
Collaborator

@YouKnowJoey YouKnowJoey Jul 1, 2024

Choose a reason for hiding this comment

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

'not_pmi_warned'? A more intuitive way to write this would be 'pmi_error_alerted = False'

Copy link
Owner Author

Choose a reason for hiding this comment

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

Good idea, but for the conditional statement, either both need to be true or both false. This seemed logical but I can look again.

message = "Private Mortage Insurance typically required with down payments less than 20 percent"
QMessageBox.warning(self, "PMI Error", message)
self.pmi_warned = True
self.not_pmi_warned = False
Copy link
Collaborator

Choose a reason for hiding this comment

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

'pmi_error_alerted = True' ; this insinuates that the error message has alerted the user.

self.ui.PMIEdit.text(),
)
calc = af_calc()
calc.process_affordability(*self.parameters)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like this! A very clean way to call the calc pass the argument to the object.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks. I think need to think about a way to guarantee the argument order.

Copy link
Collaborator

@YouKnowJoey YouKnowJoey left a comment

Choose a reason for hiding this comment

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

I'm referencing the GUI: 

  • Instead of assuming the user understands "PMI", we should write out "Private Mortage Insurance".
  • Instead of "Monthly Budget", a clearer message would be "Desired Monthly Budget".

@Josh-Voyles
Copy link
Owner Author

Good idea for the gui enhancements.

Copy link
Collaborator

@randy-shreeves randy-shreeves left a comment

Choose a reason for hiding this comment

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

Very clean. I like how you've re-constructed the affordability_calculator class. I don't have any further suggestions. I see you and Joey still have some ongoing discussions, so I won't merge it at this time.

@Josh-Voyles Josh-Voyles merged commit 87bd607 into develop Jul 2, 2024
1 check passed
@Josh-Voyles Josh-Voyles deleted the bug-fixes branch July 2, 2024 15:22
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.

3 participants