search

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

bug introduced in version 1.1 #339

Closed 2makeitwork closed 2 years ago

2makeitwork commented 2 years ago

In version 1.1, following exception is thrown when executing "client.read_area(131, 0 , 1, 1)" TypeError("unsupported operand type(s) for 'in': 'int' and 'EnumMeta'" The same command runs without issue in version 0.11.

By step in debug, it is found the when testing whether or not an integer is in enumeration s7type.Areas causes the exception in client.py. The enum "Areas" does not exist in version 0.11.

swamper123 commented 2 years ago

Could you replace 131 with snap7.types.Areas.MK ?

Enums handle stuff a bit different than the previous version, but imo this is the better way. It is more readable.

See example from the library:

        Example:
            >>> import snap7
            >>> client = snap7.client.Client()
            >>> client.connect("192.168.0.1", 0, 0)
            >>> buffer = client.read_area(snap7.types.Areas.DB, 1, 10, 4)  # Reads the DB number 1 from the byte 10 to the byte 14.
spreeker commented 2 years ago

Breaking existing solutions is very dangerous.. "Oh now I changed the code to look better", and btw oops exploded your equipment...

Senior Software Engineer. +31 6 24 69 22 97 https://gitlab.com/spreeker/ https://github.com/spreeker/ https://www.nlx.io/ https://commondatafactory.nl/

On Mon, 17 Jan 2022 at 08:03, Fabian Beitler @.***> wrote:

Could you replace 131 with snap7.types.Areas.MK ?

Enums handle stuff a bit different than the previous version, but imo this is the better way. It is more readable.

— Reply to this email directly, view it on GitHub https://github.com/gijzelaerr/python-snap7/issues/339#issuecomment-1014199217, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAOSMJFUT3E57CAHMKSTWDUWO5K3ANCNFSM5MCRDZUA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you are subscribed to this thread.Message ID: @.***>

2makeitwork commented 2 years ago

I agree with the points from both of you. The change is dangerous AND improves readability. There are better ways to do this, true, but nobody paid anything for it, right? So, relax, thank the author. If the equipment does 'explodes', that's even better. No problem, no budget.

swamper123 commented 2 years ago

@spreeker This is true. I still would expect breaking changes in a major release from 0.X.Y to 1.X.Y. (Best example: Python itself). Anyway, we should have mentioned them in the Changelogs anywhere, indeed. I have no opinion about pro or con Areas(Enum), but checking if that parameter is allowed is still important imo. Atm I haven't got a good alternative (only list or a dict maybe?) to catch all cases.

gijzelaerr commented 2 years ago

the 0.x versioning sort of implies a pre-release (although it took many years to get to 1.x), so we gave ourselves the liberty to change the API here and there. If things break for you you can still use the older version. I think we introduced this change for the 1.0 release since using an actual type is less error-prone, you can now not pass on an invalid area int value anymore.

We could support both types Areas | int, but I think it is just best to stick with the type implementation we came up with.

I consider your question answered and this issue not really a bug, just a change in the final API. Feel free to reopen if you disagree.

nikteliy commented 2 years ago

@2makeitwork They're right, but I'm on your side. @gijzelaerr I made a pull request #343 in case you change your mind.

mthuurne commented 2 years ago

Note that this change was first released in 1.1; version 1.0 didn't have the Areas enum yet.

While I agree that an enum is a nicer interface than an integer, it would be good to still accept integers for a few releases and print a deprecation warning when they're used. The type annotation could allow Areas only to push people towards the new interface.