Skip to content

Conversation

@deronnax
Copy link
Collaborator

@deronnax deronnax commented Mar 24, 2024

  • because of the >= in if self.start_time >= self.end_time:, the if self.start_time == self.end_time: can never be reached
  • ValueError is not the right error to raise, It's ValidationError. Raising ValueError throws a 500
  • The date field already checks for date validity

Nit: all the checks in the save() should be removed, they are already done in clean() (except one but I can move it into clean). In Django, all checks are done clean, and not in save. The way it's done is contrary to the django philosophy (yes theoretically a silly dev could call save with invalid data because he did not call clean()` but django is clear on it: that's his fault).
If you are OK with this proposal, I will do this.

@deronnax deronnax force-pushed the model-appointment-clean-save-improvments branch from febd76d to cec8061 Compare March 24, 2024 18:19
@adamspd adamspd merged commit 7fac162 into adamspd:main Mar 24, 2024
@adamspd
Copy link
Owner

adamspd commented Mar 24, 2024

@deronnax, you're right about everything said. I merged the pull request and the change wil be in the next release. Thanks a lot for that.

adamspd added a commit that referenced this pull request Mar 24, 2024
adamspd added a commit that referenced this pull request Mar 24, 2024
@deronnax
Copy link
Collaborator Author

Cool, thank you. Sorry, I did not notice I broke tests because they did not run on the MR. But I will know for next time.

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