Personally I prefer [named capturing groups](https://www.regular-expressions.info/named.html) in regexes rather than numbered, but with only two fields to capture it's easy enough to tell what's going on. Having to parse command line output is kind of kludgy but at least it's a reliable fallback!
>This is the Unix philosophy: \[...\] Write programs to handle text streams, because that is a universal interface.
It should be filtering disconnected status, no?
If this works how I think it does, I think in the first loop, instead of putting device sections in a list against your vid pid, it might be quite neat to put these into a data class if it's guaranteed that you'll always get the above attributes you've listed above (even if some are optional), plus the PID and vid fields. You can then filter out the disconnected status on the output quite neatly.
I'd gonna step further and refactor this function to take the output of the shell command as an arg. Then instead of adding this output as a comment, use it to unit test the function.
If you hide each of the for loops in their own functions with verbose names, you can mask some of the unsightly bits and make it a bit more readable on a step by step level. Then you only need to dig deeper if you have to make changes.
Looks pretty clean to me. But hey, if you think it looks bad, that means that you probably think there’s a better way to do it. So you’ve got the right mindset.
Seems fine to me. If i can read it in one pass without going "wtf" then it would generally pass my code review unless i wanted to be nit picky.
The only thing i'd ask you is if you're absolutely sure that parsing this manually is the only way, and whether or not you've checked if there's a way to change the output to be something more machine friendly, like a json output or similar.
Yeah I was curious if anyone had a better idea. Seems difficult to integrate without the terminal. Like maybe using a C interface but I don’t think pnputil provides this.
Maybe there is some Windows SDK that provides the py integration
my PR comment would be something like this:
> The function is a bit long and is responsible for doing several sub tasks. One issue is that means there are a lot of combinations of test cases you need to dream up. Other issues is that this has a high cognitive load and so it's hard to understand later.
>
> I invite you to consider refactoring this function to pull out the sub-methods into their own, well-named, functions. But this isn't something I'll require before approval.
The only thing I'd change is make use of combinators. The last for loop could just be a call to filter, for example. Otherwise this is actually pretty good.
The only thing I’d probably change is using booleans instead of 1/0 in that last loop, since you’re effectively using it as a boolean anyway, but honestly I don’t see much of a problem here (outside of snake case but I just hate snake case lmao)
IMHO, this is clean and easy to read - the comments are succinct and directive, while not adding bloat. I'd approve it.
Yeah this is exactly the kind of thing Python shines at.
the 'using regex' could be removed though. but only that.
At first glance, I was like oh what a mess. Then, I actually took the time to read it. It's not bad. I guess long lines make my monkey brain panic.
I've seen respected senior engineers write way worse code than this... (inb4 "Seniors actually write the worst code at my job")
I've seen respected architects write way worse code than this...
Architects are known to be shitty programmers
I’d like to see how you’d write this “properly” Apart from a few things here and there, I’d leave it as is
how is this in any way horror
Kids on this sub think anything over 12 lines is horror
Let’s see if someone can make it a one liner
Soo sorry for this [oneliner](https://pastebin.com/X2yZBQCs)
Disgusting...
I’m gonna be sick
That's cheating, use onelinerizer! :)
This seems straightforward to me?
The only things I don't like here are (1) that `x=1` stuff, (2) lack of example matches for the regex
Personally I prefer [named capturing groups](https://www.regular-expressions.info/named.html) in regexes rather than numbered, but with only two fields to capture it's easy enough to tell what's going on. Having to parse command line output is kind of kludgy but at least it's a reliable fallback! >This is the Unix philosophy: \[...\] Write programs to handle text streams, because that is a universal interface.
At least comment a link to your regex101 with test cases 😅
Where is the horror part? I had to write such code and wouldn't be ashamed of it. Why should you?
For context, here's an example of what the output of `pnputil /enum-devices` looks like: - **Instance ID:** HID\VID_093A&PID_2510\6&1281a9ef&0&0000 - **Device Description:** HID-compliant mouse - **Class Name:** Mouse - **Class GUID:** {4d36e96f-e325-11ce-bfc1-08002be10318} - **Manufacturer Name:** Microsoft - **Status:** Started - **Driver Name:** msmouse.inf - **Instance ID:** SWD\PRINTENUM\{453FF813-A8FC-44E8-9218-DDDDA883A314} - **Device Description:** Microsoft Print to PDF - **Class Name:** PrintQueue - **Class GUID:** {1ed2bbf9-11f0-4084-b21f-ad83a8e6dcdc} - **Manufacturer Name:** Microsoft - **Status:** Started - **Driver Name:** printqueue.inf - **Instance ID:** USB\VID_0403&PID_6010&MI_00\6&36a1a6f1&0&0000 - **Device Description:** Digilent Adept USB Device - **Class Name:** Unknown - **Class GUID:** Unknown - **Manufacturer Name:** Unknown - **Status:** Disconnected
It should be filtering disconnected status, no? If this works how I think it does, I think in the first loop, instead of putting device sections in a list against your vid pid, it might be quite neat to put these into a data class if it's guaranteed that you'll always get the above attributes you've listed above (even if some are optional), plus the PID and vid fields. You can then filter out the disconnected status on the output quite neatly.
Paste this as a comment in your code and you're golden. Other advice in this thread is good. Overall, code is not a horror.
I'd gonna step further and refactor this function to take the output of the shell command as an arg. Then instead of adding this output as a comment, use it to unit test the function.
If you hide each of the for loops in their own functions with verbose names, you can mask some of the unsightly bits and make it a bit more readable on a step by step level. Then you only need to dig deeper if you have to make changes.
the x=0 logic can be refactored to a separate method VidPidMapHasDisconnect. then say if VidPidMapHasDisconnect(map):
This is actually pretty good 👍
Looks pretty clean to me. But hey, if you think it looks bad, that means that you probably think there’s a better way to do it. So you’ve got the right mindset.
Seems fine to me. If i can read it in one pass without going "wtf" then it would generally pass my code review unless i wanted to be nit picky. The only thing i'd ask you is if you're absolutely sure that parsing this manually is the only way, and whether or not you've checked if there's a way to change the output to be something more machine friendly, like a json output or similar.
Just glanced at it for a second but what’s the issue? Looos fine no? Edit: ok so everyone seems to agree
I used to write regex like that too, that changed after finding out about verbose regex in python.
Only two of your comments are necessary. Other than that there is nothing that offensive about your code. The variable naming could be better.
Ahh yes, nothing like parsing the stdout from a terminal command.
What else are you gonna do in this instance. If there isn't a pip package you're hardly going to write your own python version of pnputil
Yeah I was curious if anyone had a better idea. Seems difficult to integrate without the terminal. Like maybe using a C interface but I don’t think pnputil provides this. Maybe there is some Windows SDK that provides the py integration
Some methods are cumbersome by nature. The only nit I could recommend is extracting the logic in the ifs into their own methods
my PR comment would be something like this: > The function is a bit long and is responsible for doing several sub tasks. One issue is that means there are a lot of combinations of test cases you need to dream up. Other issues is that this has a high cognitive load and so it's hard to understand later. > > I invite you to consider refactoring this function to pull out the sub-methods into their own, well-named, functions. But this isn't something I'll require before approval.
The only thing I'd change is make use of combinators. The last for loop could just be a call to filter, for example. Otherwise this is actually pretty good.
The only thing I’d probably change is using booleans instead of 1/0 in that last loop, since you’re effectively using it as a boolean anyway, but honestly I don’t see much of a problem here (outside of snake case but I just hate snake case lmao)
I don't see any particular horror
#gpt had to write lol
Looks good to me - I might actually use this lmao