-
-
Notifications
You must be signed in to change notification settings - Fork 13
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
Fix28/add an entry point #30
base: master
Are you sure you want to change the base?
Fix28/add an entry point #30
Conversation
I working on the entry point, and Make as Ready for review when i have something it work.... |
Seems awesome, thanks! 🎉 |
Hello everyone look the entry point and instalaltion are testable it can be test with by clone my repo and install it like that
You can use
Still to fixe:
Good working command exemple: image-go-nord ./directory/ ./
image-go-nord --avg ./directory/ ./
image-go-nord --ai --blur --resize 1920 1080 ./directory/ ./
image-go-nord --ai --blur ./image.jpg ./
image-go-nord --reset-palette --add '#123456' --add './palette.txt' ./image.jpg ./ breff look to work .... |
It's start looking nice and nicer! |
Look work to work. All the difuculty is push near argparse who is forced to true pass by properties setter where the logic is apply... Normaly the code is correct... i still not understand how bas64 work , then cant implement it ....
|
Could you add some tests and update the readme to add some documentation about its utilization? To me is looking quite good, I'm don't know if it's still worth maintain the image-go-nord cli project by this PR. WDYT @Wabri ? This package will come with the API library and the CLI entry point. |
Sure i can add some tests (with unittest) In general for optain a 100% test cover code i start from the frist line of code by create tests. |
Sure, let's say if we can aim for a good 70-80% of test coverage that would be super! |
I'm on it,
|
That is tested I have disable Enjoy cher
|
We are open to suggestions in terms of tests, do you have any? At the moment it's not in a CI/CD, would it be great to have it as a GitHub action for example. Maybe @Wabri you can open an issue on it if you agree |
I can give you my personnal point: I use
The design of pytest force to use plugins, then more you want options more you load plugins, and that finish to be too slow for a developper it have to do pytest really a good tool when it have html, json repport to do, it require more config, more plugin them more time, but it make good job wll. Then i recommand |
Hoooo i have forget to say. version = "1.1.1" I have increase minor version for not let PIP make a mistake during Upgrade. |
I want to point that @BugliL was working on the new cli and advance testing in this pr: Schroedinger-Hat/ImageGoNord-cli#25 He has done a impressive job on the mocking testing, see it here https://github.com/Schroedinger-Hat/ImageGoNord-cli/pull/25/files#diff-4000edb933a4a79d4e67889094ffb9fb9c57383090399e6d75e864ce1e37e8fd I've no hard opinion on this, maybe we shoud do a separate discussion on it. |
I have look the code. The use of unittest is really correct and compatible with the code on my PR. I recommand to limit the size of a PR (number of feature), because it make un-mergable result. My PR is mergable as it with only what i have describ on top of the PR creation. I can help to merge the work of @BugliL we no trouble. (But in on other PR) |
Look strange to have a CLI on both repository. I have no trouble to merge our base code. I reppeat the @BugliL PR is too big for me mergable as minor change... What is the goal of thte PR:
From my point it shuold be 4 PR to be mergable. But here we cumulate a responssability range trouble .
Hop it help. I have no trouble to help for a merge code, and have no trouble to trash my code too. |
Hi Hierosme, The difference between the gihub projects Yes I think that my PR is big, that is not a minor change, that is a big breaking change, but if you look the old code was not maintainable at all. I think the best here is to move the CLI code of this branch to the |
Hi @BugliL , hi everyone, If i understand well,
Look correct plan, The CLI i provide with it PR is 100% tested and yet require a myproject.toml, if i come with it code on We have code the same target CLI entry point without exchange about the architechture , the valuable code of my entry point is the sources / targets management. The rest is just an implementation of docs/exemple.py. From my point: (Just a personal point, nothing realtive to Truth)
Here i not understand really what is the next step ... i have no trouble to help, but the only thing i identify is the need to create a myproject.toml file into As explain before i think it will contribute to create bad experience to ImageGoNord users, the Thanks for you response, and sorry to push hard like that to merge my code. My goal is not to create a battle with the both CLI. or to denegrate you work. Regards |
Don't worry, nobody is denegrating any work, we all want to cooperate to make ImageGoNord better 😃
As mentioned before, it's just not about space but modularity. We can evolve both, the library and the client without affecting each other. If both stay in the same package, even a small change in the library would change a version in the CLI. If the CLI stay separated could be have a fixed requirement version of the library and would work even the library completely change.
In my opinion, the first objective should be the merge of part of the functionalities that you added here, specific:
The client in the |
I do agree with @BugliL , let's split things first so that we can merge the changes here then open a PR on the cli repository. |
Look a bit like a refactoring. From my point it type refactoring should be address by multiple PR and if possible from main/master branch of both repository. The Why not simply merge and schedule the refactoring as it should? We dont speack about a misstake introduse by the PR code, then for me if no error is on the code it should be merge. Yes by close #28 issue with it PR it look to introduse perturbation ... but that not invalide the issue and the PR. Schedule a refactoring by multiple PR, is a good practice because without that i'll let you choose where save my work, and let you choose the better moment to integrate it... I propose to merge, and create Issue on both project for cover the refactoring. |
I've not strong opinion too, we can use the cli in here just for the sake of test of the pip package and use the cli repository to enhance the usability of the pip package. In those cases we should tell somewhere in the docs that this cli is just for test purpose. For me no problem with the merge! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this CLI is much more than what is needed to have a [__main__.py](http://__main__.py/)
entry point. This is good if we want to use the library calling it directly but I think we should continue all CLI improvements on the other repository.
I will ask you just to add few lines in docs, so that people that will read the code or print the help could know that exists a different project with CLI code.
After this edit I think that we can create subsequent issues and go on to the other project 😄 🚀
Co-authored-by: Lorenzo Bugli <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the missing doc lines. I'm approving this PR, so we can move forward.
@Wabri if you agree we can merge this after the #33 so that I can create two different release.
Please @BugliL and @Hierosme I'll let you create the roadmap/issues for the next part !
Huge contribution, thanks again @Hierosme !
Thanks @TheJoin95 The migration to myproject.toml is a big improvement. I waiting for a merge before any change |
Please rebase, then we can merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Pleaser rebase and change the destination branch ref to be "main" |
Conver by it Pull Request
pyproject.toml
image-go-nord
entry pointUsage
Installation - Normal
pip install .
Installation - Normal with extra AI
Installation - Develloper mode
pip install -e .
Installation - Develloper mode with extra AI
Entry point