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

Extract constants and exception into a module #2287

Closed
wants to merge 11 commits into from

Conversation

MattHag
Copy link
Collaborator

@MattHag MattHag commented Feb 14, 2024

Does not change any behavior, just reducing dependencies on low level modules in order to get the device module testable.

Related #1097

@pfps
Copy link
Collaborator

pfps commented Feb 14, 2024

Is this going to conflict with PR #2265

@MattHag
Copy link
Collaborator Author

MattHag commented Feb 14, 2024

Yes, that can happen, but I think this should be merged before that. It is an important step towards cleaner and testable code in the logitech_receiver module.
I can add some comment next to your change too, but briefly: I would not move constants and Exceptions into any of the existing hidpp modules as this leads to more coupling and cross module dependencies (circular import issues we currently fight against).

The constants and Exceptions are static things, that can exist in their own module. They can be used everywhere, as they only have a dependency on common. By extracting these into their own modules it reduces the code, that relies on the hidpp modules. That's the purpose of this refactoring.

@MattHag MattHag force-pushed the test_device_1097 branch 3 times, most recently from 3abc901 to ee9dc6c Compare February 14, 2024 23:52
@pfps
Copy link
Collaborator

pfps commented Feb 15, 2024

Yes, this should be done, but after the imports fixes, as it touches the same place that the imports fixes do. You could download that PR and build on it.

@MattHag
Copy link
Collaborator Author

MattHag commented Feb 16, 2024

Yeah, it's fine. I can adapt it later.

@pfps
Copy link
Collaborator

pfps commented Feb 16, 2024

OK.. Once 1.1.11 is out we can work further on fixing the import structure and the hidpp stuff.

@pfps
Copy link
Collaborator

pfps commented Feb 18, 2024

@MattHag There are a few conflicts that need to be cleaned up.

@pfps
Copy link
Collaborator

pfps commented Feb 18, 2024

DEVICE_KIND from HID++ 2.0 is mapped to DEVICE_KIND from HID++ 1.0 in device.py and then the 1.0 version is used everywhere else. I wonder if therefore DEVICE_KIND should be put somewhere else (common?) so that it is not identified as an HIDPP 1.0-only enumeration, and the mapping itself moved into hidpp20.py.

But perhaps the best way forward is to get this PR cleaned up and merged, and then do any fine-tuning.

@pfps
Copy link
Collaborator

pfps commented Feb 20, 2024

I tried to fix the conflicts here but had to download the PR, fix the conflicts, and then create PR #2317.

@pfps pfps closed this in #2317 Feb 20, 2024
@MattHag MattHag deleted the test_device_1097 branch June 2, 2024 17:04
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.

None yet

2 participants