PyEvdi Fixes

[raldone01]

Modernize cpp code.
Rename acquire_framebuffer_cb to acquire_framebuffer_handler for consistency.
Change cpp standart to 20 for span.

Merged:

• Formatting
• Add instructions to generate compile_commands.json.
• Fix Makefile to use virtual env if activated.
• Add instructions to build python module.

[Hi-Angel]

Add linux kernel clang format.
Disable Autoformat like the kernel guidelines suggest.
Add instructions to generate compile_commands.json.
Modernize cpp code.
Fix incorrect version in test.
Add instructions to build python module.
Fix Makefile to use virtual env if activated.
Rename acquire_framebuffer_cb to acquire_framebuffer_handler for consistency.

That sounds like 8 different commits, not just one. I'm not a project member though, but typically a commit "let's refactor everything in a single changeset" would not pass review.

It also lacks details on why things have been done. E.g. Disable Autoformat like the kernel guidelines suggest — why? A little commit message would help reviewer to get in line with what you're suggesting. Add instructions to generate compile_commands.json. — here you can simplify review by shortly mentioning that it is for code completion and errors highlight in LSP-servers, and LSP is supported in all modern IDEs and text editors. You and me know what's that for, but a reviewer (or some future reader of the commit history) is not necessarily aware of that.

See also this article from HID Linux subsystem and libinput creator. "Old but gold".

[raldone01]

Add linux kernel clang format.
Disable Autoformat like the kernel guidelines suggest.
Add instructions to generate compile_commands.json.
Modernize cpp code.
Fix incorrect version in test.
Add instructions to build python module.
Fix Makefile to use virtual env if activated.
Rename acquire_framebuffer_cb to acquire_framebuffer_handler for consistency.

That sounds like 8 different commits, not just one. I'm not a project member though, but typically a commit "let's refactor everything in a single changeset" would not pass review.

It also lacks details on why things have been done. E.g. Disable Autoformat like the kernel guidelines suggest — why? A little commit message would help reviewer to get in line with what you're suggesting. Add instructions to generate `compile_commands.json`. — here you can simplify review by shortly mentioning that it is for code completion and errors highlight in LSP-servers, and LSP is supported in all modern IDEs and text editors. You and me know what's that for, but a reviewer (or some future reader of the commit history) is not necessarily aware of that.

See also this article from HID Linux subsystem and libinput creator. "Old but gold".

There were multiple commits but I squashed them into one. 🙂
Maybe I shouldn't have.
I contributed to some projects in the past that complained about too many commits.

If a maintainer chimes in and wants to get some/all of this merged I am willing to separate out some change sets but I am not doing it if merging is not on the horizon.
Creating multiple interdependent pull request is no fun.
I would rather create a big one and separate changes as they get merged.

I also figured it might be fine in this case because PyEvdi was two thirds done and I could not find any user of the module.

All fair points though.

I wonder though if you are not a maintainer why bother reviewing my code?
Do you have some use case?
You can also look at the other draft pr #423.
It is fully working I only marked it as draft not to confuse the maintainers about the order.

[Hi-Angel]

I contributed to some projects in the past that complained about too many commits.

That's odd, although I've seen such projects as well. Matrix bridges seems to follow the "squash everything upon merge" workflow. Well, maybe those developers are just not experienced enough. In the end, there are reasons for keeping commit history clean.

Creating multiple interdependent pull request is no fun.
I would rather create a big one and separate changes as they get merged.

Agree. Although, seeing how this PR haven't got a comment from maintainers for ½-year, it might be useful to note that I personally don't contribute too many changes at once to a project I didn't contribute before, because situations like this one where the contribution is being plainly ignored by maintainers are far too frequent, alas. It's often useful to send a few small changesets first to simply test if the project is even "contributable" 😊

I wonder though if you are not a maintainer why bother reviewing my code?

Well, why not, I was a bit bored and had some free time, so took a look at it. Code-review also shows up on Github profile in case a potential employer would want to look at it, so there's that as well.

Do you have some use case?

Sorry, no, I found out this project just 5 hours ago 😊

[raldone01]

If I have some time and motivation I will try to separate out the first change set. Maybe then the maintainers might engage.

Thanks for the review and insight.

Btw I use the code I submitted on my server.

[displaylink-emajewsk]

Hi. When I assigned myself I only checked if it compiles and then got busy with another project and forgot about it, sorry.
If you could separate the changes into multiple commits as @Hi-Angel suggested, I would much appreciate it.
At least the clang formatting should be separate from code improvements and C++20. Thank you.

[raldone01]

@displaylink-emajewsk Is this pr small enough now?
I am not sure if it makes sense to split off more.

[displaylink-emajewsk]

It's fine now. Great PR. I'm going to merge it. Thank you very much. :)