Refactor package to be imported as a single package #37

barretobrock · 2020-12-18T15:24:11Z

The aim of this refactoring is to ultimately have a single package to import when pip installing the package to a new environment. Currently, pip installing the package results in multiple modules being created (see issue #36 for more details)

Noting some of the major changes that took place during this refactoring:

All files that were related to the package were moved into the reolink_api directory
Package name in setup.py changed to reolink_api from reolink-api to allow for import in Python (hyphenated names result in error)
Submodule files that were originally uppercase have now been made lower case
Type hinting enforced throughout most methods & functions
- Note that float is used quite extensively, as opposed to int. This is the recommended method as int is implied in float more info here
- Also note that Dict is used over dict. This generic type will allow for better-defined dictionary types in the future, if so desired.
Added a tests directory to eventually establish a more robust testing routine for all submodules.
Added two new submodules:
- download.py - handles downloading motion video files
- motion.py - queries camera for motion events (and their video files) during a specified window

…, add tests

…ng throughout, complete first pass for logic issues

barretobrock · 2020-12-18T15:25:42Z

@Benehiko I'll make another logic pass today just to make sure there's nothing else I didn't catch. I also need to verify that all the README items were properly transferred back to this fork.

Feel free to reach out if you have any questions in the meantime.

barretobrock · 2020-12-18T19:44:24Z

reolink_api/config_handler.py



 class ConfigHandler:
    camera_settings = {}

    @staticmethod
-    def load() -> yaml or None:
+    def load() -> Optional[Dict]:


Noting here that Optional[] essentially is the same as or None

barretobrock · 2020-12-18T20:50:29Z

reolink_api/api_handler.py

@@ -69,7 +75,10 @@ def login(self) -> bool:
                print(self.token)
                return False
            else:
-                print("Failed to login\nStatus Code:", response.status_code)
+                # TODO: Verify this change w/ owner. Delete old code if acceptable.


Pointing out this change here.

barretobrock · 2020-12-18T21:15:50Z

reolink_api/recording.py

        """
        'https://support.reolink.com/hc/en-us/articles/360007010473-How-to-Live-View-Reolink-Cameras-via-VLC-Media-Player'
        Blocking function creates a generator and returns the frames as it is spawned
-        :param profile: profile is "main" or "sub"


This was unused.

Benehiko · 2020-12-18T21:43:21Z

@barretobrock Looking good! I'll go through it later this weekend and do the merge. Maybe we should change the package name to reolinkapi for consistency - the golang package is importing as this name

barretobrock · 2020-12-18T21:55:18Z

@Benehiko even better! I'm wrapping up my second & final logic pass so I'll add the package new change with that.

Restored `requirements.txt` Updated `setup.py` to include new repository url and contact details. Moved the rtsp code from `record` to `stream`. Updated project structure to make it more readable and developer friendly - moved mixins to the `mixins` package, moved handlers to the `handlers` package. Moved files not belonging to anything in particular to the `util` package. Updated `camera` class to also defer login call. Deleted unused files like `config_handler`.

Benehiko · 2020-12-19T18:01:08Z

@barretobrock I have added some changes as well to the project structure and file names. I went through the changes made and it looks good to me :)

I have added @themoosman to just review it as well

See commit 2b3e142

Benehiko · 2020-12-19T18:28:24Z

Is requirements.txt a requirement for having a library on PyPI? I had initially removed it in favor of placing all the necessary reqs in with setup.py, though if this file is needed for any another reason, perhaps I should add something in setup.py to read from this file instead of having its own list in the file? Either way, it would be easier to maintain pinned requirements in just one place over two.

For PyPi not so much, but for other developers wanting to contribute it might be a good idea. I do agree with you about having one place to maintain the requirements. Not sure if setup.py fulfills both contributor and PyPi needs. We could maybe try this suggestion on SO and just add a . to the requirements.txt file which reads from the setup.py file.

Benehiko · 2020-12-19T18:32:38Z

It might be cool to also set a prop like is_logged_in here so when we call things like _execute_command() is_logged_in would automatically be checked before running the command to ensure we're connected.

I'm not sure about whether Reolink expends any resources maintaining a connection once a user's logged in, but it does seem like a good idea to at least provide the option of deferring login. Good catch +1

Would be quite useful having a prop, the reason for the defer_login is so users' of the library can manage their object pool (say many cameras) without needing to request the camera anything over the network before object creation giving the user that freedom of choice.

barretobrock · 2020-12-19T18:44:58Z

Is requirements.txt a requirement for having a library on PyPI? I had initially removed it in favor of placing all the necessary reqs in with setup.py, though if this file is needed for any another reason, perhaps I should add something in setup.py to read from this file instead of having its own list in the file? Either way, it would be easier to maintain pinned requirements in just one place over two.

For PyPi not so much, but for other developers wanting to contribute it might be a good idea. I do agree with you about having one place to maintain the requirements. Not sure if setup.py fulfills both contributor and PyPi needs. We could maybe try this suggestion on SO and just add a . to the requirements.txt file which reads from the setup.py file.

Oh that's nice! I do agree that whatever approach should be one that most developers are already familiar with. Personally, I work mainly with requirements files, since it's easier to just swap the production requirements file with a QA version that's has more bleeding-edge versions on it. I'm not sure if this is the best approach, but in the past I've just read that file into setup.py as a list and pass that straight to install_requires, making sure to filter out empty strings and stripped lines beginning with a hash (#)

themoosman · 2020-12-19T19:14:27Z

I second keeping requirements.txt it makes it easy for developers.

themoosman · 2020-12-19T19:47:37Z

I just did some quick tests and everything looks good. LGTM

themoosman

See my comments.
LGTM

Benehiko · 2020-12-19T21:13:09Z

@barretobrock @themoosman updated pypi as well https://pypi.org/project/reolinkapi/

Bobrock added 10 commits December 13, 2020 12:41

Apply intial changes to make repo perform better as a Python module

91c92b0

Add changes from original examples file, apply find_packages to setup…

00835e3

…, add tests

Improvements to motion detection and download methods, add to examples

86117de

Version bump

7afa58a

Resolve issue when querying motion files results in empty results

856ede1

Add back pipfiles, README

84ed848

Move description read-in beneath metadata

b10c29c

Add description to new example. Swap import on original example

17bc207

Enforce lowercase standard on all submodule files, improve type hinti…

63537f9

…ng throughout, complete first pass for logic issues

Reduce redundant code, remove unused arguments

7283bd3

barretobrock commented Dec 18, 2020

View reviewed changes

Bobrock and others added 4 commits December 18, 2020 15:58

Complete second logic pass, remove underscore from package name

0a48984

Complete second logic pass, remove underscore from package name

4c4dd7d

Merge branch 'master' into develop

0fdd0df

Benehiko requested a review from themoosman December 19, 2020 17:57

themoosman approved these changes Dec 19, 2020

View reviewed changes

Benehiko merged commit e1cb9bd into ReolinkCameraAPI:master Dec 19, 2020

Benehiko linked an issue Dec 19, 2020 that may be closed by this pull request

Python installation results in split, arbitrary libraries #36

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor package to be imported as a single package #37

Refactor package to be imported as a single package #37

barretobrock commented Dec 18, 2020 •

edited

Loading

barretobrock commented Dec 18, 2020

barretobrock Dec 18, 2020

barretobrock Dec 18, 2020

barretobrock Dec 18, 2020

Benehiko commented Dec 18, 2020

barretobrock commented Dec 18, 2020

Benehiko commented Dec 19, 2020

Benehiko commented Dec 19, 2020

Benehiko commented Dec 19, 2020

barretobrock commented Dec 19, 2020

themoosman commented Dec 19, 2020

themoosman commented Dec 19, 2020

themoosman left a comment

Benehiko commented Dec 19, 2020

Refactor package to be imported as a single package #37

Refactor package to be imported as a single package #37

Conversation

barretobrock commented Dec 18, 2020 • edited Loading

barretobrock commented Dec 18, 2020

barretobrock Dec 18, 2020

Choose a reason for hiding this comment

barretobrock Dec 18, 2020

Choose a reason for hiding this comment

barretobrock Dec 18, 2020

Choose a reason for hiding this comment

Benehiko commented Dec 18, 2020

barretobrock commented Dec 18, 2020

Benehiko commented Dec 19, 2020

Benehiko commented Dec 19, 2020

Benehiko commented Dec 19, 2020

barretobrock commented Dec 19, 2020

themoosman commented Dec 19, 2020

themoosman commented Dec 19, 2020

themoosman left a comment

Choose a reason for hiding this comment

Benehiko commented Dec 19, 2020

barretobrock commented Dec 18, 2020 •

edited

Loading