-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add canned explanation about feature preview flag to deprecated(). #10167
Conversation
Thanks @uranusjr so much for the review! I really appreciate it. I'm on the road at the moment but I will try to address everything soon. |
Hello @uranusjr, I have attempted to address all of the points you raised. I am ready for the next round of review. I'm happy to further elaborate on my motivation, or to rebase/squash my commits. Thank you! |
I am a -1 on this PR as it stands -- more context at #10165 (comment). |
Hello! I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the |
Closes #10223 |
@pradyunsg and @uranusjr, do the latest commits satisfactorily address all your concerns? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor thought, generally looks good enough to me.
Oops, I need to adjust the unit tests now... |
Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com> and Pradyun Gedam <pradyunsg@gmail.com>
@uranusjr and @pradyunsg, unit tests are passing, and from my side things look ready to go. Thus I squashed my commits to tidy things up. Any final comments? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The repeated formatted_xxx = XXX.format() if foo else None
feels cumbersome, but it does the job, and functionality-wise lgtm.
Thanks @maresb! I quite like where we ended up here, functionality wise! ^>^ |
Great, thanks so much for the merge @pradyunsg! I agree that this ended well, I like the current solution much better than my original proposal, so thank you and @uranusjr for the collaboration. |
Closes #10165