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

WIP: Remove regex from discovery #42

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

Conversation

shepherdjay
Copy link

This is currently work in progress. However, I thought it would be useful to get the pull request started to get discussion going on the way I'm approaching it.

So far I've pulled out the logic that decodes the received discovery message into a separate utility function. This allowed me to test it in isolation and reduce duplicate code. As an aside SenseMe.details didn't appear to be used other than as part of this decode so I removed it from the class. If its used elsewhere let me know.

Once complete this pull will close #5

Can run command "python setup.py test" to run tests
Includes test for that decoder function
@shepherdjay
Copy link
Author

Might need to add some configuration to Codacy. "assert" statements are the proper way of referencing test statements in Pytests framework.

@TomFaulkner
Copy link
Owner

Interesting that it doesn't like that. In my code for work we assert lots of things for development purposes. I assume that is a common practice. I'll look into making codacy not care about those

@TomFaulkner
Copy link
Owner

I appreciate the tests, and for now I'll ignore the codacy issues with the asserts. I noticed that you moved the regex to a function, it definitely looks better than the in-line regex. I commented on the issue with a bunch of test strings.

@shepherdjay
Copy link
Author

Thanks for the test strings. Yeah the Regex was moved into a function to allow me to test the before and after more easily as a unit with predictable results. With the test strings I should be able to validate the current behavior and then refactor out the regex completely since we have a known separator.

@shepherdjay shepherdjay force-pushed the master branch 3 times, most recently from 41c9f15 to e0e469e Compare October 12, 2018 16:20
@TomFaulkner
Copy link
Owner

Life has been busy for me, I'm sorry I didn't get more strings to you.

Was it the initial discovery strings that you were in need of?

Also, did you get your shirt?

@shepherdjay
Copy link
Author

So what I was looking to add to testing was essentially line 859 where m equals something from the receive buffer. I was hoping to get examples of m maybe from that logging module.

I did get my shirt 😄

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.

Remove requirement on RegEx
2 participants