Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2022-06-27
10:10:25 sean-k-mooney i dont like that we have to rememeber that this is not just an az its a union of an az and a sentinal
10:10:26 gibi anyhow I go back to review the unshelve patch
10:10:51 gibi sean-k-mooney: so you would add an extra param to the call instead? like az_provided ?
10:11:01 gibi that way we can avoid the sentinel
10:11:31 gibi new_az can be None to unpin or a valid AZ name, and az_provided boolean can be set if no AZ field is provided in the REST request
10:11:36 sean-k-mooney perhaps or wrap the parmters in a class
10:12:22 sean-k-mooney i would think that az_provided would be more readable
10:13:07 sean-k-mooney so ideally even if we had a sentinil i would like use to do that test once at the top of the function and defien a local az_provided varible
10:13:27 sean-k-mooney that better expresses the intent
11:37:58 opendevreview Amit Uniyal proposed openstack/nova master: Adds validation for hw machine type in host caps https://review.opendev.org/c/openstack/nova/+/847126
11:40:36 bauzas Uggla: sean-k-mooney: gibi: sorry folks, I was cooking and lunching with my kid
11:42:00 bauzas sean-k-mooney: not sure I understand why you unlike the sentinel pattern
11:42:13 bauzas sean-k-mooney: and why you ask for a new parameter
11:42:29 sean-k-mooney bauzas: its very very error prone
11:42:38 bauzas ?
11:42:49 sean-k-mooney you need to keep the context that the vraiable is a union of a sential and the az
11:42:55 sean-k-mooney and everywhere you use it test for both
11:43:15 sean-k-mooney its very easy to miss that
11:43:17 bauzas sean-k-mooney: not if you provide a doc
11:43:24 sean-k-mooney no one reads docs
11:43:31 bauzas a doc comment I mean
11:43:36 bauzas like I said
11:43:47 bauzas :param az: (optional) blah
11:43:52 sean-k-mooney yep i hate those
11:44:29 bauzas and so, you prefer to have pypi parameters...
11:44:33 sean-k-mooney if we have a sential i would prefer use to do the test once at the top of the function and never refernce the centinal again
11:44:53 sean-k-mooney well thos are at least testable by mypy so yes
11:44:59 sean-k-mooney but that is missign the point
11:45:26 bauzas sean-k-mooney: well, if we have two tests, one for verifying a sepific value for new_az and another one for verifying to not use the parameter, I wouldn't see why it could be a problem
11:45:30 sean-k-mooney you are adding extra context that you need to keep in mind
11:45:49 bauzas sean-k-mooney: but we have done that since I'm working in Nova
11:46:01 sean-k-mooney in very limits places
11:46:16 sean-k-mooney i really dont want o see us doing this more in the code
11:46:17 bauzas lemme look at Uggla's change
11:49:01 bauzas sean-k-mooney: in general within RPC methods
11:49:18 bauzas so, I briefly looked at Uggla's code
11:49:47 bauzas I don't see why people wouldn't understand this is an optional parameter when looking at the call https://review.opendev.org/c/openstack/nova/+/831507/14..16/nova/api/openstack/compute/shelve.py#106
11:50:07 bauzas and in https://review.opendev.org/c/openstack/nova/+/831507/14..16/nova/compute/api.py#4467 Uggla added docstrings
11:51:33 bauzas but I understand your point
11:51:40 bauzas maybe we could use PEP661 then https://peps.python.org/pep-0661/
11:52:21 bauzas and using the sentinels module
11:52:39 bauzas lemme see if py3.8 supports it
11:53:09 bauzas mmm, not stdlib
11:53:46 bauzas oh my bad, PEP661 is a drafy
11:53:50 bauzas draft*
11:54:19 sean-k-mooney bauzas: its not the call site
11:54:36 sean-k-mooney its the use site within the function i find distasteful
11:54:58 bauzas you mean, the caller or the called method ?
11:55:07 sean-k-mooney the use in the called method
11:55:14 bauzas the if conditional ?
11:55:18 sean-k-mooney yes
11:55:21 bauzas hah
11:55:31 sean-k-mooney the fact you have to check the sential with an is check explictly
11:55:39 bauzas well, then we could have one single conditional that would set a value or not
11:55:55 sean-k-mooney right which is what i actully was askign for
11:56:09 bauzas would you then prefer something like
11:56:13 sean-k-mooney i didnt leave review feedback yet because i was in the midel of other thngs
11:56:48 sean-k-mooney bauzas: also https://peps.python.org/pep-0661/ is basically the same as cpp's std:optional
11:57:00 sean-k-mooney at least in inteded usage
11:57:05 sean-k-mooney so yes i would prefer that
11:57:36 sean-k-mooney that said i dont se how its used
11:58:13 bauzas sean-k-mooney: https://paste.opendev.org/show/bLmJ5uKGsufcYjXAIda3/
11:58:34 bauzas would you prefer this pattern ?
11:59:03 bauzas we only set an internal value if the parameter was provided
11:59:21 bauzas so we only check the sentinel value once at the top of the method
11:59:31 sean-k-mooney not quite
11:59:41 sean-k-mooney the problem with that is we can refernce undeined varbles
12:00:08 sean-k-mooney that actully a error in any path where new_az is not _sentinel
12:00:36 bauzas well, the reference to _sentinel is unique
12:00:49 bauzas you can check its id
12:01:21 bauzas that's why this pattern exists in Python to verify whether this parameter was called
12:01:33 sean-k-mooney https://paste.opendev.org/show/bqHsJAFVEGmRncJcERMd/
12:01:41 bauzas as, if the id of the value explicitly matches the unpassed
12:02:05 sean-k-mooney if _new_az or host: in your version is a runtime error
12:02:23 bauzas sean-k-mooney: true
12:02:30 bauzas I like your counterproposal
12:03:01 sean-k-mooney i just like have a name for the concept that we are modeling
12:03:11 sean-k-mooney and abstracting how it implemtned via the name
12:03:14 bauzas of course, "az_passed == new_az is not _sentinel" without the trailing double-dot :)
12:03:29 bauzas and with a single equal
12:03:36 bauzas but I got your idea
12:04:03 bauzas you create an always-set internal reference for knowing whether this field was set or not
12:04:15 bauzas this is a good pattern
12:04:23 bauzas and only at the top of the method
12:04:31 sean-k-mooney yep
12:04:51 sean-k-mooney you check once and define a varible at the top of the method with a meaningful name
12:05:02 sean-k-mooney and then use that later so you dont need to keep the context loaded
12:05:02 bauzas wfm
12:05:12 bauzas Uggla: ^
12:05:29 bauzas Uggla: tl;dr follow the pattern proposed by https://paste.opendev.org/show/bqHsJAFVEGmRncJcERMd/
12:05:51 bauzas but modify the first line by "az_passed = new_az is not _sentinel
12:27:52 Uggla bauzas, ok
12:44:22 gibi Uggla: fyi I'm in the middle of reviewing your patches so you will get feedback from me soon too
12:45:05 Uggla gibi, a lot of new ones ?
12:46:15 gibi Uggla: couple of nits in the code and a list of suggestions in the functional tests
12:46:45 Uggla gibi, ok I'm gonna wait for your comments.
13:00:28 gibi Uggla: posted my comments
13:00:39 Uggla gibi, ok thx
14:01:23 opendevreview Jan Hartkopf proposed openstack/nova master: add support for updating server's user_data https://review.opendev.org/c/openstack/nova/+/816157
14:03:03 opendevreview Jan Hartkopf proposed openstack/python-novaclient master: add support for microversion 2.91 https://review.opendev.org/c/openstack/python-novaclient/+/816158

Earlier   Later