-
Notifications
You must be signed in to change notification settings - Fork 58
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
Python installation results in split, arbitrary libraries #36
Comments
Update: for more clarity on my suggestion, I've forked the repo and made the structural changes I suggested. NB! The README and other root-level files (e.g., |
HI @barretobrock thank you for using the library, really appreciate your input. I will look into refactoring the library as you have mentioned, but I will need to hold back on that for a bit since it will introduce some breaking changes if someone is using the library straight from GitHub. So I'll do this once I've sorted some other changes I want to make and then bump the version to v0.1.0. |
Sounds good. Happy to help either way. |
that would be great, I wanted to remove the openCV dependency before doing that, but it seems I can't find a proper RTSP client implementation in python3. So it's been taking longer than expected. I suppose, let's just do the refactor and then I'll get rid of the dependency at a later stage. I'm open for a PR if you're willing 👍 |
Yep, I can likely submit a PR this weekend. Regarding RTSP clients, I have limited experience with them, but in the ~2 implementations I've seen the package provider will simply provide an easy way for the user to obtain the RTSP url or stream and then let the user make the decision about what client they want. Here's an example of that from Amcrest's package. I think if you decided to go this route, you could include in the examples a use case for using openCV without having to include it in the requirements. Anyway, just my 2 cents. |
That would be great 👍 Yeah in the current release I updated it to only return the image stream (using cv2) with an example of how to view the stream using cv2 (here). So the plan is just to replace the cv2 dependency with a smaller alternative and keep the example as is - meaning the return data will be the same. I have found some RTSP clients that do return a raw stream, but it would need a bit of work for it to be considered stable enough to include in this library. I have done something similar in the Go library, but it actually had a proper library to work from. |
Hi! Thanks for building this library out. I'd love to use it in some upcoming projects.
I thought I'd point out that the structure of this repo is such that installing as a python library (e.g.,
pip install reolink-api
) results in a mixed bag of libraries (api, utils, Camera, etc...) that don't seem to be able to communicate with each other too well. For instance, running the following after a fresh install:from Camera import Camera
results in a
ModuleNotFoundError
, as theutil
file isn't found and thus can't importfrom util import threaded
.If the structure of the library were to be changed to something like my suggestion below, it would likely function more like a regular python library and thus would avoid the problem of modules not being found:
(From the project root)
Also, in
setup.py
, changing theNAME
variable toreolink_api
orreolinkapi
(matching the directory name change above), as having a-
in a package name in Python can result in unexpected issues when performing a standard import.Anyway, just thought I'd bring this up in case your goal was for this project to behave more like a "standard" Python library. I'm certain there are other ways to maintain a repo than what I've suggested, so I accept that I might be wrong in my assumptions about your project. That said, if you'd like some help in moving your project in the direction I've suggested I'd be happy to help out.
Cheers,
The text was updated successfully, but these errors were encountered: