| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-05-31 | |||
| 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 | |
| 13:57:03 | sean-k-mooney | the black defaul is 88 | |
| 13:57:23 | sean-k-mooney | which works fine even on 1080 screesn or my phone | |
| 13:57:38 | sean-k-mooney | which is actully 1080 on landscpae so i guess thats the same | |
| 13:58:30 | bauzas | sean-k-mooney: changing our pep8 tool is a totally different effort than changing the limit | |
| 13:58:36 | sean-k-mooney | unless you have a 720p screen you can open two 90 char termins in a 1080 screen and still have space | |
| 13:58:51 | sean-k-mooney | bauzas: its not changing the pep8 tool by the way | |
| 13:58:59 | sean-k-mooney | its still using flake8 | |
| 13:59:00 | bauzas | sean-k-mooney: sorry I meant the formatter | |
| 13:59:22 | sean-k-mooney | but isnt of autopep8 which does minimal formating it would be black which does everything for you | |
| 13:59:58 | bauzas | sean-k-mooney: if black stays optional, I'm not opposed to it | |
| 14:00:12 | bauzas | sean-k-mooney: are you planning to make it a tox target ? | |
| 14:00:17 | sean-k-mooney | in terms of developer mental healt not having auto formating is defferntly bad for burn out | |
| 14:00:41 | sean-k-mooney | bauzas: it would have to be enforced in ci | |
| 14:00:47 | bauzas | well, I certainly had burnout conditions in the past that didn't occur because of the formatting... | |
| 14:00:48 | sean-k-mooney | otherwise there is no real point | |
| 14:01:21 | sean-k-mooney | i condiered stopping working on nova because of the effort required to get patches merged much of which was becasue of style considerations | |
| 14:01:45 | bauzas | a formatter wouldn't help, no ? | |
| 14:02:00 | sean-k-mooney | it would | |
| 14:02:13 | sean-k-mooney | auto fromating would remove debates of how to format thigns | |
| 14:02:19 | sean-k-mooney | its what the tool does and done | |
| 14:02:44 | bauzas | I'm certainly not one who debated the stylish things | |
| 14:03:16 | bauzas | for line limits, I'm not opposed to use either brackets or backslashes, eg. | |
| 14:03:24 | bauzas | or even local variables | |
| 14:03:41 | bauzas | at least, I could comment on it, but I'm not *opposed* to it | |
| 14:04:28 | bauzas | so, if you felt exhausted by those debates, I really feel your frustration and I think we should rather document the fact that we shouldn't enforce exact styling guidelines | |
| 14:05:00 | bauzas | yet again a code review issue and not a tool miss | |
| 14:05:10 | bauzas | (not saying pypià | |
| 14:05:10 | bauzas | (not saying pypià | |
| 14:05:13 | bauzas | ;) | |