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

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

Merged
merged 16 commits into from
Jul 25, 2024
Merged

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

merged 16 commits into from
Jul 25, 2024

Conversation

Novecento99
Copy link
Contributor

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
Copy link
Owner

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

snap7/util/db.py Outdated Show resolved Hide resolved
@Novecento99
Copy link
Contributor Author

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

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

@Novecento99
Copy link
Contributor Author

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

@gijzelaerr
Copy link
Owner

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

@Novecento99
Copy link
Contributor Author

done!

@gijzelaerr
Copy link
Owner

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
Copy link
Contributor Author

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

@Novecento99
Copy link
Contributor Author

Novecento99 commented Jul 17, 2024

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
Copy link
Contributor Author

let me try to improve it

@Novecento99
Copy link
Contributor Author

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

@gijzelaerr
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

snap7/util/db.py Outdated Show resolved Hide resolved
snap7/util/db.py Outdated Show resolved Hide resolved
snap7/util/db.py Outdated Show resolved Hide resolved
tests/test_util.py Show resolved Hide resolved
@nikteliy
Copy link
Contributor

tackle the test wheels osx/amd64

It is most likely not your fault; it fails occasionally

@Novecento99
Copy link
Contributor Author

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

They all completed successfully :)

Copy link
Contributor Author

@Novecento99 Novecento99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

regex use substituted this code section

@Novecento99
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@gijzelaerr gijzelaerr merged commit e0aaed2 into gijzelaerr:master Jul 25, 2024
68 checks passed
@gijzelaerr gijzelaerr added this to the 2.1 milestone Jul 25, 2024
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.

3 participants