Skip to content
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

Make it more useable out of the box #1

Merged
merged 22 commits into from
May 7, 2023
Merged

Make it more useable out of the box #1

merged 22 commits into from
May 7, 2023

Conversation

lrq3000
Copy link
Collaborator

@lrq3000 lrq3000 commented May 2, 2023

Awesome idea and project, thank you very much for sharing your work!

I stumbled by chance on it, and even though I am not working in video compositing, I am an experienced Python developer, so I thought I would help in making this app more useable out of the box for others.

Here are the changes:

  • Modernize packaging to PEP517 standard (build isolation, build requirements, dependencies specified in the same file, pyprojects.toml).
  • Add a videogestalt binary script entry that is installed in the local environment.
  • Allow to specify output as an argument.
  • Can be used as a Python module too.
  • Minor documentation improvements: added a Description section, a License section, and embedded the example image (future-proofing).

The app is ready to be deployed, with python -sBm build . and then twine dist/.

Also if you would be open to having a maintainer, I would be happy to maintain the project. I am very unlikely to add new core features, but I can manage PRs, bugfixes and deployment. I can for example automate deployment to PyPi with an automated test deployment on TestPyPi using a GitHub Action, when a GitHub Release is created, as I did in my other projects.

lrq3000 added 7 commits May 2, 2023 00:49
…cutable script entry + integrate example gestalt image inside the repository

Signed-off-by: Stephen L. <[email protected]>
…lepath) + videogestalt can be used as a Python module

Signed-off-by: Stephen L. <[email protected]>
Signed-off-by: Stephen L. <[email protected]>
@lrq3000
Copy link
Collaborator Author

lrq3000 commented May 2, 2023

PS: The app works on Windows too, so it likely works on MacOS too.

@lrq3000
Copy link
Collaborator Author

lrq3000 commented May 6, 2023

@eobrain Did you have some time to have a look at this PR? FYI, anyone can create any repository on PyPi with any name, so anybody can create a videogestalt repository right now. If you want and don't have time to manage this repo right now, I can book videogestalt in the PyPi repository for you and transfer rights to you when you will have a pypi account.

It's just that your app is so amazing and the name so perfectly representing what the tool does that I think it would be a shame if we get scooped the PyPi repo name :-)

@eobrain
Copy link
Owner

eobrain commented May 7, 2023

Hey, this is great! I just reviewed the code diffs and they look good. I'm going to just test in the branch before I merge.

And it looks like you would be a good maintainer. I'll add you after I do the merge.

And feel free to go ahead and book videogestalt in the the PyPi repository repo. Thanks.

@lrq3000
Copy link
Collaborator Author

lrq3000 commented May 7, 2023 via email


Licensed under the Mozilla Public License 2.0

[1]: https://github.com/eobrain/videogestalt/master/resources/vespa-commercial-gestalt.gif
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a particular reason for pulling the example GIF output into the repo? If not, I suggest leaving it in its original position in GitHib static images.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes usually when an image is meant to be included in the README and not just in the static website, I prefer to include them in the repository to be more future proof, as URLs of static sites may change or be down at any point in the future (it already happened in the past). There is also another pragmatic reason, it's because I don't know where the image is stored currently (I cannot find the source branch from which the image is pushed to GitHub pages?).


```bash
./gestalt.py -i test.mp4 -g
pip install --upgrade -e .
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me (using Python 3.9.2 on Linux) this failed with

ERROR: File "setup.py" not found. Directory cannot be installed in editable mode: /home/eobrain/src/lrq3000
(A "pyproject.toml" file was found, but editable mode currently requires a setup.py based build.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmm I guess your pip version is not updated to the latest, as it should be fixed since pip versions of last year. Nevertheless, I have implemented a workaround now, please retry with my updated PR and let me know if this works :-)


```bash
./gestalt.py -i test.mp4 -g
pip install --upgrade -e .
```
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest adding the commands to run the videogestalt command from the local development version

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the git cloning, this is the only command that should be needed. Now that I fixed the ERROR: File "setup.py" not found, this should work as expected :-)

lrq3000 added 13 commits May 7, 2023 11:05
… message to reflect new command name

Signed-off-by: Stephen L. <[email protected]>
…mit continuous integration unit test to Python v3

Signed-off-by: Stephen L. <[email protected]>
@lrq3000
Copy link
Collaborator Author

lrq3000 commented May 7, 2023

Ok I have worked some more on the potential retrocompatibility issues with the modern python packaging standards, and I also added a simple unit test that will need to be improved later to be more robust but it will do for now :-)

At this point the package should be ironed out, please let me know if it installs fine for you too :-)

@eobrain eobrain merged commit 5875be2 into eobrain:main May 7, 2023
@lrq3000
Copy link
Collaborator Author

lrq3000 commented May 7, 2023

Thank you @eobrain for the fast review and merge, and for adding me as a maintainer! I'm going to setup continuous integration and deployment asap :-)

@lrq3000
Copy link
Collaborator Author

lrq3000 commented May 7, 2023

Also I will make the first PyPi release after all tests pass, unless you want me to postpone the release :-)

@eobrain
Copy link
Owner

eobrain commented May 7, 2023

Welcome! And thanks for making this project so much better

Yes, please go ahead and make a PyPi release

@lrq3000
Copy link
Collaborator Author

lrq3000 commented May 7, 2023

@eobrain You are very welcome, the new Python packaging system is marvelous but it has a bit of a learning curve, I'm very happy to help :-)

I'm sorry, I will need a bit more of your help :-/ I don't have access to the repository settings, so I can't create environment secrets, such as for coverage and automated PyPi upload, you will either need to add them yourself (I can give you the instructions or even the tokens directly by e-mail), or give me additional permissions. If the latter, I never did so myself, but the author of another repository recently did it for me and told me he had to make the project into an organization first to be able to make me an admin.

Please let me know what you prefer and I'll help the best I can :-)

@lrq3000
Copy link
Collaborator Author

lrq3000 commented May 7, 2023

Let's discuss in #3 (PR to prepare for the first release) for the rest :-)

@eobrain eobrain mentioned this pull request May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants