| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-05-31 | |||
| 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 | ;) | |
| 14:20:22 | dansmith | sean-k-mooney: bauzas: the other day we were talking about something related to file-backed memory support in libvirt/nova | |
| 14:20:47 | dansmith | I don't remember for what, but I wrote something to get support started for the guy that added that support, which never got merged: https://review.opendev.org/c/openstack/devstack/+/574792 | |
| 14:21:00 | kashyap | sean-k-mooney: What is the "black default is 88"? | |
| 14:21:10 | dansmith | it just got some "is this still interesting" action recently, so... is it worth getting that merged? | |
| 14:21:19 | bauzas | kashyap: that means by default black generates lines of 88 chars | |
| 14:21:37 | bauzas | dansmith: reloading the context | |
| 14:21:39 | kashyap | I don't know what is "black" here. Maybe I'm being too dense | |
| 14:21:47 | bauzas | kashyap: https://pypi.org/project/black/ | |
| 14:22:02 | kashyap | Ah, it's a tool! | |
| 14:22:06 | kashyap | Thanks | |
| 14:23:34 | bauzas | kashyap: and there is blue, a fork of black https://pypi.org/project/blue/ | |
| 14:23:57 | bauzas | see, we diverted from a very interesting ping from dansmith | |
| 14:24:01 | kashyap | bauzas: Yeah, I recall seeing that in passing; th | |
| 14:24:01 | dansmith | black's coding style is so ugly | |
| 14:24:05 | kashyap | s/th/thx/ | |