damiafuentes / DJITelloPy

DJI Tello drone python interface using the official Tello SDK. Feel free to contribute!
MIT License
1.22k stars 490 forks source link

Query about a few design decisions #49

Closed paulbuis closed 4 years ago

paulbuis commented 4 years ago

I come from an academic background that does not involve robotics or embedded systems and I'm hoping to use your code as a basis for something to share with my students. When I write Python-based libraries to share with students, I do a lot more type checking and parameter validation than you do. Is this a performance issue or are you assuming the users of your code are less likely to do stupid stuff?

I know if I write code that I intend to use just myself, I am much less fanatical about that sort of thing, but when I turn code over to students I want to make sure they get a bit more feedback and never have to wade through stack dumps from errors thrown deep inside some library my code uses.

I see some folks run your code on devices like a Raspberry Pi and your code works with swarms. I'm running the code on a Windows laptop with an 8th gen Intel Core i7 and only working with a single drone, so I'm not seeing performance issues that may be real in other useage scenarios.

For example, you have your code annotated with types so "mypy" can do static type checks, but I don't trust my students to run "mypy" on the combo of their code that uses my library, so I do things like:

def foo(self, x: int):
   x = int(x)
   if x<0:
      ...
   ...

or even

def foo(self, x: int):
   try:
       x = int(x)
   except:
       raise ...
   finally:
      if x < 0: ...
   ...

I expect to file some pull requests and will be more than happy to add more bulletproofing, but before I submitted the PRs, I wanted to make sure I wasn't going to include things that you had purposely excluded because of the potential performance impact. I could make some the extra checking optional with code like so the extra checking was an opt-in thing:

def foo(self, x: int):
   if self.paranoid:
      x = int(x)
      try:
         ...
  ...
M4GNV5 commented 4 years ago

Actually there was static type checking implemented using a special function decorator. However as it proxied all functions it messed up the IntelliSense suggestions in vscode / vscodium. I thought i could solve that problem using type annotations built into python so my course members (mainly age 12-16) could have both IntelliSense suggestions and type errors which is why I changed everything to type hints in cf29c181bb14604f1809042755b78b86fc300f10. It turns out the type hints are not enforced :sweat_smile:

I had a chat with a friend a few days ago about the topic, he managed to quickly throw code together and created a decorator similar to the one that was implemented in DJITelloPy, but using the types from annotations instead of named decorator arguments. In fact there are already multiple libraries on pypi providing a similar functionality, e.g.: enforce-typing. Using the decorator from the library is probably the cleanest and simplest solution.

paulbuis commented 4 years ago

The static type checker I’ve always used with Python is “mypy” which works off of type hints. Type hints are not enforced at runtime, but if you are writing library code, they do at least tell you if you are being internally consistent. Your code is not. For example, in the code that checks on the UDP reply to a command, where the expected values are “ok” or “error” but an alternative is that the packet got lost and we time out instead, the appropriate Python return time would be Optional[bool] where the value None would indicate a timeout, True corresponds to an “ok” and False corresponds to an “error” response. To work with an Optional, the normal thing to do is handle the None case with “if x is None:” or the bool case with “if x is not None:” The Optional construct is common in many languages, most typically to prevent null pointer exceptions in languages like Java where it became traditional to return null (essentially the same as a Python None) under error conditions instead of a reference to an actual object.

Users of a library’s public API may fail to type check their code, so when I create a library I do explicit type checks right way to cut down on requests for help from users (typically students in my course or the courses taught by my colleagues) who are calling my library with arguments that are not the correct type. Normally, I don’t release code for others to look at until it has passed 3 code quality checkers: “mypy,” “pylint,” and “flake8.” They all check for different kinds of things, although there is some overlap.

From: Jakob Löw notifications@github.com Sent: Wednesday, August 5, 2020 8:11 PM To: damiafuentes/DJITelloPy DJITelloPy@noreply.github.com Cc: Buis, Paul 00pebuis@bsu.edu; Author author@noreply.github.com Subject: Re: [damiafuentes/DJITelloPy] Query about a few design decisions (#49)

Actually there was static type checking implemented using a special function decorator. However as it proxied all functions it messed up the IntelliSense suggestions in vscode / vscodium. I thought i could solve that problem using type annotations built into python so my course members (mainly age 12-16) could have both IntelliSense suggestions and type errors which is why I changed everything to type hints in cf29c18https://nam05.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdamiafuentes%2FDJITelloPy%2Fcommit%2Fcf29c181bb14604f1809042755b78b86fc300f10&data=02%7C01%7C00pebuis%40bsu.edu%7Cdea3659defbb49643bf508d8399d38d6%7C6fff909f07dc40da9e30fd7549c0f494%7C0%7C0%7C637322694716404244&sdata=kjVL0psAtxZsWVnML3ma2mtzXT1LaU8BKALFWMu48Hw%3D&reserved=0. It turns out the type hints are not enforced 😅

I had a chat with a friend a few days ago about the topic, he managed to quickly throw code together and created a decorator similar to the one that was implemented in DJITelloPy, but using the types from annotations instead of named decorator arguments. In fact there are already multiple libraries on pypi providing a similar functionality, e.g.: enforce-typinghttps://nam05.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpypi.org%2Fproject%2Fenforce-typing%2F&data=02%7C01%7C00pebuis%40bsu.edu%7Cdea3659defbb49643bf508d8399d38d6%7C6fff909f07dc40da9e30fd7549c0f494%7C0%7C0%7C637322694716414239&sdata=w8fbsHkqEM7MB%2BdhDM6%2BrbiVFaMHSbS4APjwNZ5mB3A%3D&reserved=0. Using the decorator from the library is probably the cleanest and simplest solution.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://nam05.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdamiafuentes%2FDJITelloPy%2Fissues%2F49%23issuecomment-669608626&data=02%7C01%7C00pebuis%40bsu.edu%7Cdea3659defbb49643bf508d8399d38d6%7C6fff909f07dc40da9e30fd7549c0f494%7C0%7C0%7C637322694716414239&sdata=f%2F7p1zLyD0odmA3k25uMmQ5kSFG%2Fgf5mIJQY5mIpq2c%3D&reserved=0, or unsubscribehttps://nam05.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAA7ULYZTCIRWBCYCUN4OHALR7HYJZANCNFSM4PV2IQMA&data=02%7C01%7C00pebuis%40bsu.edu%7Cdea3659defbb49643bf508d8399d38d6%7C6fff909f07dc40da9e30fd7549c0f494%7C0%7C0%7C637322694716424235&sdata=oF6iHMfkbiM3tB8KxQzbwnb0qnOfbMzWyR57lVsoRzY%3D&reserved=0.

M4GNV5 commented 4 years ago

Hey,

As you can see above i added a few commits in order to make the library easier to use and more consistent. ec4ff7b makes mypy happy. Running mypy now reports: Success: no issues found in 5 source files . Also commit ba56ba8 re-added a decorator for type safety checks of function parameters.

I hope this enables you to use the library in a lecture. Otherwise feel free to implement missing features and open a pull request.