Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add key beeps to switch2477S #37

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

freak3dot
Copy link
Contributor

@freak3dot freak3dot commented May 28, 2023

Code changes:

  • Add a mixin for key beeps so that it can be used by both dimmer2477D and switch2477S.
  • DRY up the DefaultMsgHandler since we need it for both KeyBeepMixin and switch2477S.

Notes:

  • I had trouble figuring out your file naming conventions as I saw underscores, camel case, and all lowercase. I went with all lowercase as it seemed to be the most prevalent.
  • I also had trouble figuring out your indention convection as I saw mixed tabs and spaces. I used spaces as that seemed more common.
  • There are more things that could be moved into the handlers folder, but that is beyond the scope of adding the key beeps and I did not want to overstep. Also, I only have switch2477S. So, my ability to test changes to other devices is limited.
  • My Python is quite rusty as it has been years since I have written Python. I welcome constructive criticism.

Resolves #36

Ryan Johnston added 2 commits May 28, 2023 09:08
…in and switch2477s at the same time. Use absolute imports because that seems to be what makes jython happy.
@berndpfrommer
Copy link
Collaborator

First: thank you for this PR.
Unfortunately I no longer have any hardware to test this code since I sold my house several years ago.
I'm not much of a Python expert and learned about mixins from your code, thanks!
Yes, there is no consistency in tabs vs spaces, it's a mess. I will clean it up to meet PEP 8 after your PR has been merged.
The one thing that puzzles me about your PR is the import statements. Do you really need to specify the full path (python.mixins.xxx)? I really don't like the "python" in the front since it suggests that this is a module that belongs to the python language. @pfrommerd put something in the Readme that says that the import statements should not include "python" at the beginning. I think the import also works without the "python." in front. At least I can instantiate an object and invoke some methods on it.

@freak3dot
Copy link
Contributor Author

I actually spent a long time trying to figure out the imports. I've never worked on a Jython project before. I usually have a virtualenv in my past projects. The imports only seemed to work with the python workspace. We could eliminate it if we put everything in the same folder. It looks like the content of the folder is currently pretty mixed anyways.

Or, I could submit another pull request that just copies and pastes the relevant functions which is a little redundant, but consistent.

@berndpfrommer
Copy link
Collaborator

I'm a bit puzzled by the fact that you need the "python." since all the other imports work without it (using openjdk 11.0.19 python 3.10.6 on Ubuntu 22.04). Could you please double check that you cannot do without the "python."? For me your class works with or without the "python." prefix on the import.
If the sub directories are the cause of the problem, then I'd rather you eliminate them as you suggested. I may be guilty of code duplication myself but I generally avoid it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2477S Needs key beeps
2 participants