#59 feature/improve_genconfig

Closed
LecygneNoir wants to merge 0 commits from feature/improve_genconfig into develop
Owner

This feature aims to fix #55 to have a working genconfig, and improve the genconfig process

This feature aims to fix #55 to have a working genconfig, and improve the genconfig process
LecygneNoir force-pushed feature/improve_genconfig from 51614a4bfe to 304a32853d 4 months ago
LecygneNoir force-pushed feature/improve_genconfig from 304a32853d to 1a006f3b6c 4 months ago
Zykino approved these changes 4 months ago
Zykino left a comment

Good overall. If I understand well we are not forced to do python -m prismedia, I shouldn’t have changed all the README for the complex syntax when an easier one is here 😅.

prismedia/genconfig.py Outdated
logger = logging.getLogger('Prismedia')
def overwrite_or_not(question):
Zykino commented 4 months ago

We may also use this function to ask if the user is sure it’s video is an mp4 file, maybe something for the --with* options too like "Your configuration asks for a Thumbnail but none was found. Do you want to overight the configuration?".

If you too think the function can be reused maybe we should place it in utils.py?

Also maybe rename it to "ask_overwrite" or "is_overight_authorized".

We may also use this function to ask if the user is sure it’s video is an mp4 file, maybe something for the `--with*` options too like "Your configuration asks for a **Thumbnail** but none was found. Do you want to overight the configuration?". If you too think the function can be reused maybe we should place it in utils.py? Also maybe rename it to "ask_overwrite" or "is_overight_authorized".
LecygneNoir commented 4 months ago

Good idea, it's done!

Good idea, it's done!
prismedia/genconfig.py Outdated
for f in files:
copyfile(join(path, f), f)
final_f = f.replace(".sample", "")
overwrite = True
Zykino commented 4 months ago

Even if you have a call to ask for the result, it is better to set the safest option here. And the safest is also the usual default for booleans.

Even if you have a call to ask for the result, it is better to set the safest option here. And the safest is also the usual default for booleans.
LecygneNoir commented 4 months ago

I agree, but unforunately I find nothing elegant for that.

If we use False here, it means we have to duplicate the code to apply the option if there is no file. It force us to do "if overwrite then copyfile" and another "else then copyfile" after the if exists.

I try to deal with it but at the end, as the call is mandatory if file exists, I find better to not duplicate more code and simply sue True as default.

If you have a more elegant way to write it, it would be a pleasure!

I agree, but unforunately I find nothing elegant for that. If we use False here, it means we have to duplicate the code to apply the option if there is no file. It force us to do "if overwrite then copyfile" and another "else then copyfile" after the if exists. I try to deal with it but at the end, as the call is mandatory if file exists, I find better to not duplicate more code and simply sue True as default. If you have a more elegant way to write it, it would be a pleasure!
LecygneNoir commented 4 months ago

In fact you next comments is the perfect solution 😍

I'll let you solve the thread if you think it ok? Or tell me if you see better alternatives ;-)

In fact you next comments is the perfect solution 😍 I'll let you solve the thread if you think it ok? Or tell me if you see better alternatives ;-)
prismedia/genconfig.py Outdated
if exists(final_f):
overwrite = overwrite_or_not(final_f + " already exists. Do you want to overwrite it?")
if overwrite:
Zykino commented 4 months ago

I would better have a "happy path" in the code with an "if exists(final_f) && not overwrite_or_not("..."): continue"

I would better have a "happy path" in the code with an "if exists(final_f) && not overwrite_or_not("..."): continue"
LecygneNoir commented 4 months ago

Okay! I am totally unfamiliar with this syntaxt of calling my own function inside a if (which is stupid from my brain, as exists IS a function 🙄) but it's totally possible and THE elegant way to do I searched to avoid the "true by default" you noticed when reviewing 🎉

Thanks for the suggestion!

Is it better for you like that?

Okay! I am totally unfamiliar with this syntaxt of calling my own function inside a if (which is stupid from my brain, as `exists` IS a function 🙄) but it's totally possible and THE elegant way to do I searched to avoid the "true by default" you noticed when reviewing 🎉 Thanks for the suggestion! Is it better for you like that?
prismedia/genconfig.py Outdated
overwrite = overwrite_or_not(final_f + " already exists. Do you want to overwrite it?")
if overwrite:
copyfile(join(path, f), final_f)
Zykino commented 4 months ago

Don’t you have to do "join(path, final_f)" too?
Maybe do the join before creating the final_f variable.

Don’t you have to do "join(path, final_f)" too? Maybe do the join before creating the final_f variable.
LecygneNoir commented 4 months ago

The join get the file from the "examples/xxxx" directory, so it's mandatory.

I have to create final_f to remove the .samples from the examples/ directory, but I do not want to rename original files to avoid accidental overwrite if someone play with copy/paste manually 😅

Perhaps I may create a original_f that contains both the directory and the file, but not sure it really worht it?

The join get the file from the "examples/xxxx" directory, so it's mandatory. I have to create `final_f` to remove the .samples from the examples/ directory, but I do not want to rename original files to avoid accidental overwrite if someone play with copy/paste manually 😅 Perhaps I may create a original_f that contains both the directory and the file, but not sure it really worht it?
Zykino commented 4 months ago

Maybe I am just confused about what the values of path, f and final_f are.
To me f and final_f hold the same value, except for ".sample" that is removed from final_f.
f is a filename (no path) of successive file present in the "config" dir.
path contains the path to the "config" dir.

Oh ok, so you are doing what in shell would be cp /path/f ./final_f. Doesn’t it assume the current path is the one of the program? Does it work if I start the program from any folder while "prismedia" is in the path?

Maybe I am just confused about what the values of `path`, `f` and `final_f` are. To me `f` and `final_f` hold the same value, except for ".sample" that is removed from `final_f`. `f` is a filename (no path) of successive file present in the "config" dir. `path` contains the path to the "config" dir. Oh ok, so you are doing what in shell would be `cp /path/f ./final_f`. Doesn’t it assume the current path is the one of the program? Does it work if I start the program from any folder while "prismedia" is in the path?
LecygneNoir commented 4 months ago

path contains the path to the module installed via pip concatened with "config", so when you install prismedia via pip, it get the file from inside the package, and it copy it to the local directory where you are running prismedia-init.

As currently there is no "config directory" for prismedia, all config files are mandatory in the directory where you run prismedia, so it seems logical that genconfig create files here too.

genconfig already worked like that before, but should be reworked if we add a config directory option for prismedia I agree.

`path` contains the path to the module installed via pip concatened with "config", so when you install prismedia via pip, it get the file from inside the package, and it copy it to the local directory where you are running prismedia-init. As currently there is no "config directory" for prismedia, all config files are mandatory in the directory where you run prismedia, so it seems logical that genconfig create files here too. genconfig already worked like that before, but should be reworked if we add a config directory option for prismedia I agree.
Zykino commented 4 months ago

I agree that this is a change of behavior and we already have a plan for this one. We will do it later.

I agree that this is a change of behavior and we already have a plan for this one. We will do it later.
LecygneNoir added 1 commit 4 months ago
Poster
Owner

Thanks for the review!

In fact, the python -m syntax is more for development point of view.

I learn a bit about that when working on this feature with genconfig in fact :D

But basically, thanks to the __init__.py file in the prismedia directory, python interpreter see it as a module, so when you clone the repository and go inside, by using python -m prismedia you tell python to use prismedia as a Module, and it takes first the module in your local directory.

If you have no "prismedia" directory locally, then it's the module installed by pip that is used.

So you are able to test your code this way, without having to build a full package.

And by using python -m prismedia.genconfig you call the genconfig module inside prismedia, it's possible only if the module has a the if __name__ == '__main__': part inside.

So you may use python -m prismedia.utils for example, it'll call the file, but fail as there is nothing to launch inside.

Thanks to the [tool.poetry.scripts] in the pyproject.toml file, we are able to tell pip that the package provides theses scripts as shortcut to avoid the python -m syntax for non-dev users.

So I guess I'll indeed have to rollback that in README unfortunately, I didn't know that at the moment I reviewed your MR sorry :-/

Thanks for the review! In fact, the `python -m` syntax is more for development point of view. I learn a bit about that when working on this feature with genconfig in fact :D But basically, thanks to the `__init__.py` file in the prismedia directory, python interpreter see it as a module, so when you clone the repository and go inside, by using `python -m prismedia` you tell python to use prismedia as a Module, and it takes first the module in your local directory. If you have no "prismedia" directory locally, then it's the module installed by pip that is used. So you are able to test your code this way, without having to build a full package. And by using `python -m prismedia.genconfig` you call the genconfig module inside prismedia, it's possible only if the module has a the `if __name__ == '__main__':` part inside. So you may use `python -m prismedia.utils` for example, it'll call the file, but fail as there is nothing to launch inside. Thanks to the `[tool.poetry.scripts]` in the pyproject.toml file, we are able to tell pip that the package provides theses scripts as shortcut to avoid the `python -m` syntax for non-dev users. So I guess I'll indeed have to rollback that in README unfortunately, I didn't know that at the moment I reviewed your MR sorry :-/
LecygneNoir added 1 commit 4 months ago
LecygneNoir requested review from Zykino 4 months ago
Zykino commented 4 months ago
Poster

Ok, still a bit fuzzy to me but I understood the idea. Don’t worry about the README, at least not in this PR.

Ok, still a bit fuzzy to me but I understood the idea. Don’t worry about the README, at least not in this PR.
LecygneNoir closed this pull request 4 months ago

Reviewers

Zykino was requested for review 4 months ago
This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.