Code cleanups/refactoring

[paravoid]

Per the comment on #493, I'm opening this issue to discuss a pending cleanup that I'm working on. This is in an effort to a) improve code hygiene and b) fix bugs in the process. PR #493 above has:

• Make this a proper Python package (moving files around + ~15 lines of change)
• Add a test suite with a battery of tests (no-op) + tox.ini

On top of it, I have the following:

• Cleanup/rewrite/refactor of almost every file (code structure)
• Bunch of bugfixes
• Improving human-readable output (logging etc.)
• Standardizing on the syntax (PEP8, black etc.)
• Comments/documentation

Via linting and code testing, hopefully this will also ensure (via all of the above) that bugs don't get introduced in the future :)

You can see the work so far in my GitHub fork.

There are a couple more modules pending -- I wanted to wrap this up this week and then open issues/PRs, but I'm opening it a bit early given the feedback on the issue above :)

[kueblc]

To the first points, this is a single purpose application, not a general framework or library. I don't think it's appropriate to convert into a package.

I also don't see why you renamed files unless there is a specific functional issue being addressed here. Renaming without a very good reason is additional maintenance overhead.

I will not merge a rewrite of every file at this point. As I mentioned there are a number of outstanding branches and a refactor going on behind the scenes. I wish you had started a discussion before doing all of this work...

Logging is part of the ongoing refactor/redesign.

We are not PEP8 and thus not black compatible. Notably, we use tabs, % operator for formatting strings, and don't split arrays into one element per line if not necessary. #330 #415

You are welcome to maintain your own fork of course, but unfortunately it doesn't seem like it fits entirely with project philosophy/style and other things happening here. We can discuss further down the line if there are individual parts which need improvement.

[paravoid]

To the first points, this is a single purpose application, not a general framework or library. I don't think it's appropriate to convert into a package.

This is not as much a package, as it is moving them to individual modules. The reason is so that one can do from smarthack import registration etc. Notably, this is the only way the test suite can import the code and to be able to test it - hence why I bundled it up. (This is typical for Python single purpose applications that are comprised of multiple files.)

In the future it could also allow running a single executable that runs all the different parts (e.g. in the asyncio/Tornado event loop) rather than running os.system("mqtt.py") and os.system("pkill -f smartconfig.py"). I can experiment with that and let you know where I end up.

I also don't see why you renamed files unless there is a specific functional issue being addressed here. Renaming without a very good reason is additional maintenance overhead.

The original motivation was for the same reason:

>>> from smarthack import fake-registration-server
  File "<stdin>", line 1
    from smarthack import fake-registration-server

Granted, some of them are cosmetic (mq_pub_15 would work), but given we have the opportunity while we move files around I thought I'd take it (I suppose 15 stands for some older protocol version or something). What kind of maintenance overhead are you fearing of here?

I will not merge a rewrite of every file at this point. As I mentioned there are a number of outstanding branches and a refactor going on behind the scenes. I wish you had started a discussion before doing all of this work...

Sorry about that... Happy to rebase on top of your work though. The codebase in the refactor branch is in an excellent point right now: lots of bugs fixes, homogeneous, clean, entirely lint clean, fully type-hinted, 81% code coverage etc. Setting up CI would be the natural next step. It'd be a pity to waste all that!

We are not PEP8 and thus not black compatible. Notably, we use tabs, % operator for formatting strings, and don't split arrays into one element per line if not necessary. #330 #415

That's pretty idiosyncratic... PEP8 is a core Python standard -- pretty much all code bases adhere to it these days. The point is to avoid having a spaces vs. tabs discussion in every project and has been fairly successful! black takes it one step further.

As a data point, it was extremely for me hard to even understand, let alone start contributing to the codebase -- and I'm quite used to looking at third-party codebases.

You are welcome to maintain your own fork of course, but unfortunately it doesn't seem like it fits entirely with project philosophy/style and other things happening here. We can discuss further down the line if there are individual parts which need improvement.

I'd prefer to have as many of my changes merged upstream as possible. Do you think there's a way to find some common ground here?

[kueblc]

In the future it could also allow running a single executable that runs all the different parts (e.g. in the asyncio/Tornado event loop) rather than running os.system("mqtt.py") and os.system("pkill -f smartconfig.py").

This is part of the refactor being worked on.

Granted, some of them are cosmetic (mq_pub_15 would work), but given we have the opportunity while we move files around I thought I'd take it (I suppose 15 stands for some older protocol version or something).

Seems like you have your own vision what this software should look like. Again, you are welcome to maintain your own fork. 15 is the upgrade command over Tuya's MQTT protocol.

That's pretty idiosyncratic...

Be that as it may, this is our style.

PEP8 is a core Python standard -- pretty much all code bases adhere to it these days.

That's not a good reason, just because everyone else does it doesn't make it right

The point is to avoid having a spaces vs. tabs discussion in every project and has been fairly successful! black takes it one step further.

There is no discussion. We use tabs.

As a data point, it was extremely for me hard to even understand, let alone start contributing to the codebase -- and I'm quite used to looking at third-party codebases.

Because of the tabs? 😆 It is perfectly valid Python

I'd prefer to have as many of my changes merged upstream as possible. Do you think there's a way to find some common ground here?

I would hope so, but I ask that you do not try to take over the project with your own style and organization ideals. If you have specific functional issues you'd like to address and can keep the diff small and easy to understand that would make it quick to review and merge.

[paravoid]

Great to hear about your upcoming refactor! Could you perhaps share more about your ideas or plans? I'm curious to hear how you're thinking of approaching this!

You've mentioned a couple of times that I am welcome to make my own fork. I'm (obviously) aware of that, but repeating it makes me feel a bit dismissed and like I'm overstaying my welcome here. I'm happy to have a discussion and figure out a way to collaborate, but if you feel there are no chances of that and I'm more of a nuisance than help, I can go away.

This is not a tabs vs. spaces issue by the way -- I have zero interest in having that conversation. This issue is about having a healthy codebase that one can change and contribute to easily and with confidence, to find and identify bugs and avoid regressions. There are a number of code health issues right now that have resulted in real bugs. Not blaming anyone -- it's natural for a codebase of this size, and to have e.g. str/bytes bugs in a codebase that has gone through a py2/py3 transition. FWIW, pylint scores the development branch at -4.78/10. Ignoring the mixed-indentation check (which is not a "tabs" check) bumps that to 3.17/10. flake8 identifies 242 issues, without counting W191 "indentation contains tabs". Type hints revealed a number of other issues as well during the process of adding them.

IMHO, these should be addressed structurally, with modern coding practices that would ensure this continues to be the case: that includes test suites & regression checks, linters, CI etc. That's what this effort was about :)

Tooling like this is invaluable in making improvements and identifying bugs, but it doesn't play well if your code is as idiosyncratic as this codebase is. In other words, yes, "because everyone else does it" doesn't make it necessarily right, but it makes significantly easier to leverage an entire ecosystem, one that is built around what everyone else is doing!

Concretely, if you're working on a refactor yourself, the test suite can help identify any regressions. I'd be happy to take your branch and run it through the suite and pinpoint any issues found, if that's helpful. The suite itself is pretty standalone, with the only requirement being that the code is importable.

[jjtParadox]

If I may offer my two cents:

I think it's awesome that you have a grand vision for this project, and it's obvious you want to help make this project be as rock solid as possible with pylint, tests, CI, community standards, etc.

However, I think all of that might be viewing this project to be bigger than it really is.

My (admittedly minimal) experience with code standards and CI tools is that they do indeed make maintaining large projects with dozens or hundreds of regular contributors much less frustrating. They probably make a ton of lives easier and have probably prevented hundreds of critical-or-worse bugs from being introduced in important software.

CI can even work for small projects where the maintainers (or just the one dev) are comfortable with configuring and operating it. However, I feel gains are not quite as high compared to what large, very active projects gain from CI.

Tuya-convert is small. Its primary use case is rather ephemeral: a user downloads it once, runs it a few times, and then it's never run again. At worst, a bug in tuya-convert just means a user can't easily flash a cheap object. An issue is submitted and a fix comes shortly after.

However, it does have a high maintenance cost separate from the software due to the nature of hardware reverse engineering. Because of this, the one (or more?) maintainer might not have the time or resources to learn how to operate CI, and the gains CI and large code standards provide might not make sense in the context of other maintenance costs. It needs to serve the maintainer, not burden.

Right now, despite it being messy, tuya-convert's code format and standards do serve the maintainer.

Still, there is room to improve. Perhaps the direction to go with this discussion is learning the maintainer's frustrations with the current code base, or small incremental changes that stick close to methods and standards the maintainer already uses but still improve the stability and quality of the code (i.e. a manually run suite of tests that doesn't require a refactor).

/2¢

[kueblc]

Hi @paravoid, I am sorry it has taken a while to get back to you. I would like to apologize that in our previous interactions I may have come across defensive. For some context I would like you to understand I have nothing against you or your code, that we have had a recent loss in the family and other personal matters. I really do appreciate how much thought and care you have put into your code.

I think that @jjtParadox has hit on a few points much better than I had been able to, understanding that while it might not be the picture of modern programming practices, it is indeed small project with ephemeral use and by design it is simple and slow changing.

Small incremental changes are definitely the way to go, something that would be very quick and easy to review and get used to. I do want to improve the quality of the codebase, and would be thrilled to have help along the way, but some modern programming practices do not, as @jjtParadox suggests, serve me, as they are either not applicable to a project this small, or I simply do not agree with them.

I also want to apologize and clarify that my suggestions to maintain a fork were not meant to dismiss you, rather that if you indeed can do better than me and devote more time, energy and resources than I can provide, you may well be better suited to maintainership. I have put a lot into this project, but I understand there are always better programmers and those better equipped to take on the demands of maintainership.

I stepped up because I saw a need, because I care about firmware freedom, and because I have extraordinary experience with the inner workings of Tuya devices. This is what I bring to the table, and if you can do it better, I invite you to, with goodwill. I accept that it may be more than you signed up for, in which case I hope that you consider helping me make incremental, planned and discussed changes to improve tuya-convert.