-
Notifications
You must be signed in to change notification settings - Fork 10
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
Remove requirement on RegEx #5
Comments
I would be willing to take this up, do you have an example string you are currently running the regex against so I can write a test? |
That's a good question. I can see where the three are, but as for the string, I don't think I have an example of that at the moment. Do you have a device to run it on? If so, you could run a few commands and log the output (might have to add logging for that) to see them. Tests are good, I've been meaning to add unit tests for some time! If you don't have a device, I can get back to you in a couple of days with some strings. |
I don't have a device myself, but can get started on the code changes I would intend to make and just insert whatever test strings you get back to me with into the test. Since you don't have a test framework yet for this project am I good implementing in pytest or would you prefer the built in unittest? I can do it in either. |
I have been meaning to try pytest. Briefly looking at the docs for it I think that will be fine. There isn't a linter config, but I am using black's autoformatting. (https://github.com/ambv/black) |
Sorry about the delay on the test strings! Here is a bunch!
|
Apologize for the misunderstanding, do you have an example of the raw messages that are received on the buffer? The regex currently does a match for messages that include DEVICE;ID which makes me wonder if other types of messages are received. So this line here: Maybe modify to add something that logs that message like this:
|
Looking back at the code, you're right in that the examples I gave above aren't sufficient. The ones I gave are status updates, which don't have the same content as some of the other responses. The lines above are output from what is now line 1293, the The lines I think you are looking for is the discover and discover_single (which are sadly duplicate of one another.) I'll get back to you with output from those. |
Yeah looking to just get what the socket sees. So that |
Here is a working refactored function example. Not sure if this is more readable than regex, let me know your thoughts:
|
Sorry to not get back to you, as I said in the PR comments. The code looks good, I'll have to compare it to a string from the fan. It will probably be next week when I can get them to you. |
The regex call, which appears in both discovery functions, is unnecessary, switch to split on ;
The text was updated successfully, but these errors were encountered: