| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-05-31 | |||
| 10:01:56 | gibi | ack | |
| 11:43:08 | stephenfin | gibi: sean-k-mooney: If you do normalization, I see no reason not to apply that normalization to stable branches also | |
| 11:43:29 | sean-k-mooney | hum we coudl i guess | |
| 11:43:37 | gibi | including downstream? :) | |
| 11:43:40 | sean-k-mooney | im just fixign a few long lines on a black patch | |
| 11:43:54 | sean-k-mooney | gibi: it woudl get imported downstream if we did it upstream | |
| 11:43:59 | sean-k-mooney | but also yes | |
| 11:44:02 | gibi | sean-k-mooney: ahh good point | |
| 11:44:04 | stephenfin | gibi: yeah, what sean-k-mooney says | |
| 11:44:58 | stephenfin | I mean, in theory black's "slow" formatter checks to make sure the AST is identical before and after so there will be no functional differences | |
| 11:45:20 | sean-k-mooney | yep that is true for the most part | |
| 11:45:35 | sean-k-mooney | also its faster tehn autopep8 | |
| 11:45:43 | sean-k-mooney | or flake8 | |
| 11:45:45 | gibi | there will be a sh*tload of merge conflicts in open reviews but that is just one-time pain | |
| 11:46:19 | gibi | and easy to resolve by rebase, run black, push | |
| 11:54:06 | sean-k-mooney | the patch is pretty bix but the list of files where i actully need to make code chages by intoducing new varbailes is pretty small | |
| 11:54:08 | sean-k-mooney | https://paste.opendev.org/show/be8e3ZYGZeIjFv8uOcvz/ | |
| 11:54:31 | sean-k-mooney | well that or () to put things onto a new line | |
| 11:55:00 | sean-k-mooney | black was able to handel almost all of it its slef but some of our mock names are kind of long | |
| 11:57:14 | sean-k-mooney | over all the code is looking quite readable after the format | |
| 11:57:51 | sean-k-mooney | stephenfin: you will like htat it seam to be quite close to your coding style already | |
| 11:59:08 | stephenfin | sean-k-mooney: yeah, I've been slowly forcing myself to move to that style of late since it seems to be getting pretty widespread now | |
| 11:59:25 | sean-k-mooney | same but mainly because of you | |
| 11:59:49 | sean-k-mooney | to premempt you asking me to change things in nits :) | |
| 12:00:05 | gibi | :) | |
| 12:00:06 | sean-k-mooney | got to make you work harder for those -1 stats :) | |
| 12:00:48 | gibi | sean-k-mooney: did you moved to 88 char line limit as the paster suggests? | |
| 12:01:00 | sean-k-mooney | ya | |
| 12:01:13 | sean-k-mooney | so the reason i did that is to mvoe to 79 with black | |
| 12:01:23 | sean-k-mooney | i would need to add project.toml | |
| 12:01:29 | sean-k-mooney | which i could do | |
| 12:01:36 | sean-k-mooney | but that seam like an even bigger change | |
| 12:01:51 | gibi | I would add -l 79 to the black command line as a stopgap | |
| 12:01:55 | sean-k-mooney | once i have this passing for 88 i can add a scond commit for 79 | |
| 12:02:17 | sean-k-mooney | if we want 79 we could sqaush them | |
| 12:03:00 | sean-k-mooney | gibi: i have configured flak8 for 88 too in this patch | |
| 12:03:15 | sean-k-mooney | so im not removing flak8 or hacking | |
| 12:03:35 | sean-k-mooney | they will still enforce checks but im makign them compatibale with black | |
| 12:03:36 | gibi | ahh I see | |
| 12:03:59 | gibi | I'm OK with the two commit to review but I would like to squash before we merge | |
| 12:04:25 | sean-k-mooney | yep so the reason im going to do two commits is 1 we can decide if we prefer one vs the other | |
| 12:04:52 | sean-k-mooney | and two i know i will likely have to resolve more long nested calls for 79 | |
| 12:05:15 | sean-k-mooney | i.e. obj.sub_obj.long_function_name.... | |
| 12:06:16 | sean-k-mooney | our convention of mock_full_long_function_name.return_value.assert_called_once_with( | |
| 12:06:28 | sean-k-mooney | sometimes is over 88 when all on one line | |
| 12:06:35 | gibi | I see | |
| 12:09:32 | sean-k-mooney | lol i just opend the libvirt driver unit test and emacs warned me of performance issues with large files | |
| 12:10:27 | sean-k-mooney | to be fiar to ti it is 32k lines long | |
| 12:10:57 | sean-k-mooney | we really do need to decompsoe the libvirt driver into more moduels and split up the unit test file eventually | |
| 12:11:08 | sean-k-mooney | its just a lot of merge conflicts | |
| 12:12:23 | sean-k-mooney | its one of those things that we just need to decide to do at teh end or start of a cycle | |
| 12:14:32 | gibi | sean-k-mooney: I totally agree. pycharm simply dies on that file :D | |
| 12:21:28 | sean-k-mooney | i dont know if im the only one that does this but one trick i have adopted recently for fixing pep8 issue is starting form the end fo the file and working backwards | |
| 12:21:45 | sean-k-mooney | that way the earlier errors dont change line number | |
| 12:22:18 | gibi | good idea :) | |
| 12:22:34 | gibi | if we merge the black patch you can forget this trick :) | |
| 12:22:50 | sean-k-mooney | hehe mostly true | |
| 12:22:54 | sean-k-mooney | self.task.compute_rpcapi.finish_revert_snapshot_based_resize_at_source.assert_called_once_with( | |
| 12:23:01 | sean-k-mooney | that is an example of something it cant fix | |
| 12:24:44 | opendevreview | ribaudr proposed openstack/python-novaclient master: Microversion 2.91: Support specifying destination host to unshelve https://review.opendev.org/c/openstack/python-novaclient/+/831651 | |
| 12:24:51 | sean-k-mooney | i am manually fixign those like this | |
| 12:24:54 | sean-k-mooney | mock_finish_at_source = ( | |
| 12:24:56 | sean-k-mooney | self.task.compute_rpcapi.finish_revert_snapshot_based_resize_at_source | |
| 12:24:58 | sean-k-mooney | ) | |
| 12:25:00 | sean-k-mooney | mock_finish_at_source.assert_called_once_with( | |
| 12:40:02 | opendevreview | ribaudr proposed openstack/nova master: Allow unshelve to a specific host https://review.opendev.org/c/openstack/nova/+/831507 | |
| 12:47:49 | opendevreview | Balazs Gibizer proposed openstack/osc-placement master: Support microversion 1.39 https://review.opendev.org/c/openstack/osc-placement/+/828545 | |
| 12:48:05 | opendevreview | Balazs Gibizer proposed openstack/osc-placement master: Support microversion 1.39 https://review.opendev.org/c/openstack/osc-placement/+/828545 | |
| 12:50:20 | Uggla | sean-k-mooney, pep8 from the end of file, good idea. | |
| 12:50:53 | opendevreview | Balazs Gibizer proposed openstack/nova stable/xena: Add service version check workaround for FFU https://review.opendev.org/c/openstack/nova/+/831174 | |
| 12:51:40 | Uggla | gibi, a black patch ? To get rid of formatting ? | |
| 12:51:50 | gibi | Uggla: yepp | |
| 12:52:08 | sean-k-mooney | well more make a tool do it | |
| 12:52:26 | Uggla | gibi, oh, that will be really cool. | |
| 12:54:50 | gibi | sean-k-mooney: even the best hero needs help from a sidekick \ | |
| 13:00:48 | sean-k-mooney | one thing i dont like but i dont see a way to avoid is i have to #noqa: E501 lambdas | |
| 13:01:11 | sean-k-mooney | i.e. if a lamda defition does not fit on one line i have to ignore the line lenght | |
| 13:01:31 | sean-k-mooney | since there is not a valid way to break it up that black wont undo | |
| 13:02:07 | sean-k-mooney | there might be a config setting i could tweak in a followup but for now im just ignoring the lenght check in that specific case | |
| 13:03:17 | sean-k-mooney | the other option is to allow lamdas to be named but we block that currently | |
| 13:03:30 | sean-k-mooney | so i cant just pull it out into a varible or similar | |
| 13:03:52 | sean-k-mooney | and pulling them out into private fucntiosn is more then i want to do in this patch | |
| 13:04:16 | sean-k-mooney | im really trying to keep the manual changes as small as possible | |
| 13:04:45 | sean-k-mooney | we can always go back and fix that later by looking for the #noqa comments | |
| 13:07:59 | gibi | yeah, I think the official suggestion is that if it does not fit into a line then it should be def | |
| 13:08:13 | gibi | but I agree not do change everything in one go | |
| 13:44:16 | bauzas | sean-k-mooney: gibi: fwiw, I loudly replied to the E501 thread | |
| 13:45:26 | bauzas | tl:dr: this is a at least a bikeshed and maybe a pandora box that will generate a lot of efforts for something needless | |
| 13:45:37 | opendevreview | Merged openstack/nova stable/ussuri: [CI] Install dependencies for docs target https://review.opendev.org/c/openstack/nova/+/839813 | |
| 13:48:53 | sean-k-mooney | bauzas: perhaps but im still going to finish my nova black patch | |
| 13:49:25 | sean-k-mooney | bauzas: dealing with formating it perhaps the thing i hate most about working on nova other then the dowstream paperwork | |
| 13:50:01 | bauzas | sean-k-mooney: take it as you wish, but I won't accept any change touching this limit | |
| 13:50:31 | bauzas | this will also break style consistency between projects and some people using terms with 80 chars will see a difference | |
| 13:51:07 | bauzas | while the other way is just because people feel useless to have 80 chars on a 1440 display | |
| 13:51:28 | bauzas | this is maybe useless, but changing the default has a cost I'm not ready to pay | |
| 13:52:17 | bauzas | and I wish our contributor energy would be better used to some other things but just a stylish nitty modification | |
| 13:52:45 | bauzas | pro-tip : 80-char limit allows you to open multiple files in parallel, that being said | |
| 13:54:16 | bauzas | btw. sean-k-mooney you wanted me to review some os-vif changes | |
| 13:54:28 | bauzas | that looks to me better use of my worktime | |
| 13:56:44 | sean-k-mooney | bauzas: i can reduce it down to 79 then and keep the limit | |