monome / druid

terminal interface for crow
GNU General Public License v3.0
35 stars 16 forks source link

asyncio serial impl, optional config files, fix logging #38

Closed csboling closed 5 years ago

csboling commented 5 years ago

Fixes #12 , fixes #14

I apologize that this is a lot of stuff at once, I can try and wrangle it into more digestible increments if desired. Tested on Windows and OSX, haven't had a chance to test Linux yet.

simonvanderveldt commented 5 years ago

Ohw, wow that's a lot of changes :scream:

For starters: Thanks for the effort!

I apologize that this is a lot of stuff at once, I can try and wrangle it into more digestible increments if desired.

Yeah, it's definitely a good idea to split this into separate PRs :)

Did a quick scan, some high level feedback, might sound a bit harsh, but I do think these things need to addressed:

I guess a more generic comment would be that if someone is planning or working on larger changes it's probably a good idea to create an issue first to discuss those changes.

tehn commented 5 years ago

wow! curious to look into the design ideas here. thanks for your efforts-- i'll be able to write more this evening.

csboling commented 5 years ago

TLDR: This is basically all to support more advanced use cases, the issues it addresses can probably be solved with more localized changes, and I don't want reviewing it to be a distraction from more pressing work. I'll try to split out some elements worth keeping into smaller patches.

That's a lot/way too many classes. I really doubt we need them There's no need for the nested package structure and large amount of modules

Not the first time I've been told this! Mostly I get anxious when files get over a few hundred lines. Smaller files are often somewhat easier to merge I think, but it can definitely be more cognitive load to figure out what lives in which file. Classes here I'm mostly trying to use to decouple functionality but I'm willing to be convinced that the boundaries are too granular.

I don't really see a need for a config file, especially at the moment where there is barely any functionality and we should be focusing on fixing the basics.

I can see a couple sides to this. The principal advantage of runtime configuration is increased flexibility : I believe these initial options give the ability to customize aspects of the capture pane behavior that were being discussed recently on #13. This can also be empowering to users, since it's easier to make local changes to high level configuration than it is to modify source or put together a pull request. I included style options here because some forum discussion pointed out that the text is difficult to read on some terminal backgrounds, which may be a difficult problem to find good one-size-fits-all solutions for.

You can of course change your terminal background color instead, which avoids needing to support / document this configuration, though often that's a system-wide change and this is a simpler example of appearance customization. This seems like the main trade-off to me: flexibility of operation and promotion of more data-driven code structure vs documentation and validation burden. I guess my feeling is that every application wants for runtime configuration eventually, and this can be designed for early or painful to add later. Or not so bad to add later or totally unnecessary, also definite possibilities!

the whole custom reimplementation for the serial connection or what exactly is happening in druid/io/serial/device.py but what issue are we trying to solve? Is it really an issue and if it is is it worth taking on so much complexity/code to fix it?

These changes as I see them address a couple of concerns:

if someone is planning or working on larger changes it's probably a good idea to create an issue first to discuss those changes.

Totally fair! Often I find discussion of structural stuff like this hard to articulate without having some code in hand. This all basically grew out of my own harebrained explorations and I thought some of the structural changes might be nice to upstream in order to be working from the same codebase for both simpler and more complex use cases. With the benefit of sleeping on it I agree that this is probably overengineered for druid's primary functionality.