ReolinkCameraAPI / reolinkapipy

Reolink Camera API written in Python 3.6
GNU General Public License v3.0
207 stars 46 forks source link

Refactor package to be imported as a single package #37

Closed barretobrock closed 3 years ago

barretobrock commented 3 years ago

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:

barretobrock commented 3 years ago

@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.

Benehiko commented 3 years ago

@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 commented 3 years ago

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

Benehiko commented 3 years ago

@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 2b3e142fe500a9b71ba57639d418b109b89bf648

Benehiko commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

themoosman commented 3 years ago

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

Benehiko commented 3 years ago

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