Skip to content

MonitorField - Change default to None when the field is nullable #577

Merged
foarsitter merged 1 commit intojazzband:masterfrom
gmcrocetti:monitor-default-none-when-nullable
Mar 22, 2024
Merged

MonitorField - Change default to None when the field is nullable #577
foarsitter merged 1 commit intojazzband:masterfrom
gmcrocetti:monitor-default-none-when-nullable

Conversation

@gmcrocetti
Copy link
Copy Markdown
Contributor

@gmcrocetti gmcrocetti commented Jul 19, 2023 •
edited
Loading

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 this 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 at 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 changed the title feat(monitor): Change default to None when the field is nullable MonitorField - Change default to None when the field is nullable Jul 21, 2023
@foarsitter
Copy link
Copy Markdown
Contributor

foarsitter commented Mar 21, 2024

We should plan a deprecation path don't you think @gmcrocetti?

@gmcrocetti
Copy link
Copy Markdown
Contributor Author

gmcrocetti commented Mar 21, 2024 •
edited
Loading

We should plan a deprecation path don't you think @gmcrocetti?

Yes, we should. Is there any major release in the near sight ?
Do you have other suggestion ?

@foarsitter
Copy link
Copy Markdown
Contributor

foarsitter commented Mar 21, 2024

My cup of tea: A DeprecationWarning when the behavior is encountered and an environment variable to enable the new behavior so we can release 4.5, removing both when 5.0 is released?

@foarsitter foarsitter added this to the 4.5.0 milestone Mar 21, 2024
@gmcrocetti
Copy link
Copy Markdown
Contributor Author

gmcrocetti commented Mar 21, 2024 •
edited
Loading

Humm...I initially liked the idea but I'm not so sure after getting my hands dirty. What if we instead split this new behavior in two steps:

  1. Raise a deprecation warning - as you said - for 4.5.0 in this existing PR;
  2. We keep the original issue and target it for 5.x.x - I open a new PR containing the original proposal.

I believe it will be better cause we keep the codebase clean and also don't fall into the trap of "remembering" to remove "dead code" added by the feature flag check.

WDYT @foarsitter ?

@gmcrocetti gmcrocetti force-pushed the monitor-default-none-when-nullable branch 2 times, most recently from a57784c to bb8ed40 Compare March 21, 2024 22:52
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 21, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.57%. Comparing base (6e7158b) to head (22015b8).

Additional details and impacted files 
@@           Coverage Diff           @@
##           master     #577   +/-   ##
=======================================
  Coverage   98.56%   98.57%           
=======================================
  Files           6        6           
  Lines         768      772    +4     
=======================================
+ Hits          757      761    +4     
  Misses         11       11

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gmcrocetti gmcrocetti force-pushed the monitor-default-none-when-nullable branch 3 times, most recently from 8994d6a to b5035b2 Compare March 21, 2024 22:59
@gmcrocetti
chore(monitor-field): Add deprecation warning when the field is null …
22015b8 
…and no default is provided. The new behavior will be introduced in a next major release
@gmcrocetti gmcrocetti force-pushed the monitor-default-none-when-nullable branch from b5035b2 to 22015b8 Compare March 21, 2024 23:06
foarsitter
foarsitter approved these changes Mar 22, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@foarsitter foarsitter left a comment

Choose a reason for hiding this comment

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

Very well @gmcrocetti, perfect solution!

@foarsitter foarsitter merged commit fe1d0c6 into jazzband:master Mar 22, 2024
@gmcrocetti gmcrocetti mentioned this pull request Mar 22, 2024
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@foarsitter foarsitter foarsitter approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

4.5.0

Development

Successfully merging this pull request may close these issues.

2 participants

@gmcrocetti @foarsitter