update to work with openevse 3.0.2dev #7
Conversation
a-u-s-t-i-n
commented
Mar 21, 2020
a-u-s-t-i-n
commented
- protocol version 4.0.1
- use json by default (kept the xml parser)
- added checksum check
- protocol version 4.0.1 - use json by default (kept the xml parser) - added checksum check
|
@a-u-s-t-i-n This PR re-introduces the issue that #4 Fixed. That's not entirely surprising since there was embarrassingly no testing at all previously. I've spent a couple days cleaning up the repo and adding tests for the v1 version of the firmware that my charger is currently running, so if you'd like, you should be able to re-make your changes to the current version and make sure those tests pass (new tests for v3 would also be great!). I may try to upgrade my firmware if my hardware will support it, but that may take some time. |
|
No worries. I can add this into the current version. I thought about adding the tests last time but it seemed like too invasive an addition -- particularly since I couldn't test the v1 responces. I'm trying to figure out how to run the test rig: I haven't used pytest before and I keep getting an error: fixture 'requests_mock' not found Do you know where requests_mock should come from? |
a-u-s-t-i-n
changed the title
|
You should be able to use |
a-u-s-t-i-n
changed the title
|
Thanks -- I didn't think to consider pip. I've added in some tests: I didn't get every test you had, but I think there is enough there to cover the basics. Let me know if you disagree... I think it is ready to merge. |
| if response is None: | ||
| response = re.search('\\>\\>\\$(.+)\\<p>', content.text) | ||
| return response.group(1).split() | ||
| return self._parseResult(content.text) |
There was a problem hiding this comment.
The type hint on line 85 indicates that _send_command should return a list of strings, but your change is violating that contract - it now returns a string if the parser is XML, but if it is using the JSON parser and runs into an error, it could return None.
|
trying to apply this PR locally to test it will nt apply to the current development branch |
Can you be a little more specific as to the commands you're running and the errors you're seeing? |
|
I grabbed the raw patch and applied with git am 7.patch it fails to apply because setup.py has been removed from the repo. |
|
to be more specific in the error
|
|
@ausil , it's actually failing for multiple reasons. Can you just check out the branch and merge into a test branch? Starting at master: |
|
Let me know if there is anything else -- thanks for working with me to get it into shape. |
|
@ausil I'm not able to replicate the patch failure. In particular, the following works for me (in a new directory): When I try to follow your commands, I get: after the checkout of that branch. I also don't understand the point of those last two commands: does the --no-ff merge do something useful or necessary? |
This reverts commit 2b8944f.
|
I put the "import setup" back in. The poetry tests in github failed when I removed it and passed when it was back. I didn't set these up -- I guess they came for free when I forked from miniconfig. |
This was probably a lack of clarity on my part - you don't need to delete the import line, you need to delete the entire file. If you delete the entire file, poetry won't try to use it, it will install using the pyproject.toml file. If setup.py is there and broken, however, it seems like it tries to use it and fails. I think if you fix that, this looks good to go. |
No worries. setup.py is gone, and I was able to install locally with poetry. Thanks again! |
