| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-06-27 | |||
| 10:08:56 | sean-k-mooney | well i prefer it to this | |
| 10:09:13 | sean-k-mooney | the ovo way at least work with in and is encapulated | |
| 10:10:04 | gibi | I create an ovo class with predefined fields, and then I have to check at each access if the field exists, that seem against the fact that we predefine the fields | |
| 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 | |