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

feat(monitor): Change default to None when the field is nullable #576

Closed
wants to merge 1 commit into from
Closed

feat(monitor): Change default to None when the field is nullable #576

wants to merge 1 commit into from

Conversation

gmcrocetti
Copy link
Contributor

@gmcrocetti gmcrocetti commented Jul 19, 2023

Problem

I recently came across a non-intuitive behavior, IMHO. The MonitorField has its default value set to now even though marked as nullable. Please, I'm open to hearing the opinion case it's is expected behavior.

Solution

After catching up the discussion in #100 I decided to implement it. The discussed solution is to set its default value to None when null=True.

If might break backward compatibility the edge case of someone marking the field as null=True and not expecting default to be None.

Commandments

  • Write PEP8 compliant code.
  • Cover it with tests.
  • Update CHANGES.rst file to describe the changes, and quote according issue with GH-<issue_number>.
  • Pay attention to backward compatibility, or if it breaks it, explain why.
  • Update documentation (if relevant).

@gmcrocetti gmcrocetti marked this pull request as ready for review July 19, 2023 22:26
@gmcrocetti gmcrocetti closed this by deleting the head repository Jul 19, 2023
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.

1 participant