gijzelaerr / python-snap7

A Python wrapper for the snap7 PLC communication library
http://python-snap7.readthedocs.org/
MIT License
643 stars 245 forks source link

better parsing to avoid bugs when variables name contain whitespaces #529

Closed Novecento99 closed 1 month ago

Novecento99 commented 2 months ago

hi,

I'm working with the db layout import, to access as a dictionary the data.

This pull request regards variables with spaces in their name, which currently breaks the parsing. With this lines of code the parsing in more generic, taking the last element as the type, the first as index and the remaining as the name

thankyou

gijzelaerr commented 2 months ago

can a variable contain a space?? sounds like a problem :)

Novecento99 commented 2 months ago

can a variable contain a space?? sounds like a problem :)

industrial stuff is gonna industrial stuff ahah ¯_(ツ)_/¯

Novecento99 commented 2 months ago

I'm sorry this is my first pull request, how do I implement your review? thanks

gijzelaerr commented 2 months ago

just commit to the same branch you used initially and push, it should reflect in this PR.

Novecento99 commented 2 months ago

done!

gijzelaerr commented 2 months ago

the perfect PR would also include a unit test that shows this change works as intended. are you willing to give that a shot also?

Novecento99 commented 2 months ago

yeah of course! thankyou for your patience. I'm gonna try!

Novecento99 commented 2 months ago

pushed it!

I just realized though that compatibility with multiple consequential whitespaces should be implemented... ex "testVar (twowhitespaceshere) withTwoWhitespaces" will be saved as "testVar (onewhitespacehere) withTwoWhitespaces"

Novecento99 commented 2 months ago

let me try to improve it

Novecento99 commented 2 months ago

I guess it could be more elegant, but it works!

gijzelaerr commented 2 months ago

can you have a look at the failing tests? The pre-commit hook and mypy check are failing. You can also run the tests locally with make tox or make mypy (I think).

Novecento99 commented 2 months ago

well I'm having issues trying to run it locally, but based from the feedback of the online checks it should be fine now

Novecento99 commented 1 month ago

hi, waiting for the workflows to be approved thankyou

I developed on my local fork also the compatibility of multiple variables with the same name (it's possibile in case of structs), but better to wait to finish this first merge I think,

Novecento99 commented 1 month ago

well I'm actually struggling with these jobs :( I pushed a last commit hopefully correcting the pre-commit hook job, but I have no idea how to tackle the test wheels osx/amd64.. I'm sorry I need a bit of an help

nikteliy commented 1 month ago

tackle the test wheels osx/amd64

It is most likely not your fault; it fails occasionally

Novecento99 commented 1 month ago

Hi, I've ran the workflow jobs on my fork

They all completed successfully :)

Novecento99 commented 1 month ago

I'm really not able to takle that failing check. it returns:

______________________ TestClient.test_as_copy_ram_to_rom ______________________

self = <test_client.TestClient testMethod=test_as_copy_ram_to_rom>

    def test_as_copy_ram_to_rom(self) -> None:
        response = self.client.as_copy_ram_to_rom(timeout=1)
>       self.client.wait_as_completion(1100)

a timeout that I don't think is due to my changes

Novecento99 commented 1 month ago

hi, all checks have been passed.. is it going to be merged? thankyou