WPI: CLI-argument-#1

remace07 commented

2022-12-14 10:17:00 +01:00

Collaborator

changed arguments parsing to argparse use
issue fix #1

changed arguments parsing to argparse use issue fix #1

remace07 added 4 commits

2022-12-14 10:17:01 +01:00

Fix HLS protocol terminology in the code #12 a6780c88be

- versions => renditions
- resolutions => variants
- ranges and/or chunks => segments
- version index => master playlist
- other index => media playlist url

For now, the CLI has not been updated with this terminology, only the
code.

🩹 use argparse library for parsing CLI arguments dba143a597

✅ add tests for cli parser behaviour 4015531fe0

♻️ wrapped parser functions in a Parser object e340767b7a

Barbagus commented

2022-12-14 13:32:34 +01:00

Collaborator

Thanks @remace07!

I am a bit confused why the the "Fix HLS..." commit shows as part of that branch. I am not familiar with Gitea and therfore I find it difficult to see the diff against stable without seeing all those changes as well. Maybe @freezed can share some git wizardry here :)

In terms of code structure, how about keeping the "cli" things in __main__.py if they do not need testing, and the rest (like the parser) in a cli.py module ?

# file __main__.py

from . import cli

...

def main():
    parser = cli.Parser
    ...

Let's talk about it, for my part I prefer the separation of concerns rather than one class <=> one module.

Also, more command line arguments are most likely going/comming with #11

Thanks @remace07! I am a bit confused why the the "Fix HLS..." commit shows as part of that branch. I am not familiar with Gitea and therfore I find it difficult to see the diff against `stable` without seeing all those changes as well. Maybe @freezed can share some git wizardry here :) In terms of code structure, how about keeping the "cli" things in `__main__.py` if they do not need testing, and the rest (like the parser) in a `cli.py` module ? ```python # file __main__.py from . import cli ... def main(): parser = cli.Parser ... ``` Let's talk about it, for my part I prefer the _separation of concerns_ rather than _one class <=> one module_. Also, more command line arguments are most likely going/comming with #11

Barbagus commented

2022-12-14 13:36:10 +01:00

Collaborator

Also I will push the creation of tests/__init__.py and the pytest configuration in the pyproject.toml before we push this.

Just so testing it has it's own moment of glory in history.

Also I will push the creation of `tests/__init__.py` and the _pytest_ configuration in the `pyproject.toml` before we push this. Just so _testing_ it has it's own moment of glory in history.

Barbagus commented

2022-12-14 13:54:03 +01:00

Collaborator

Also I will push the creation of tests/__init__.py and the pytest configuration in the pyproject.toml before we push this.

Just so testing it has it's own moment of glory in history.

I just saw that you used unittest for testing. Thats fine to start I guess... it has been ages that I did not code any python unittests (or any python actually).

> Also I will push the creation of `tests/__init__.py` and the _pytest_ configuration in the `pyproject.toml` before we push this. > > Just so _testing_ it has it's own moment of glory in history. I just saw that you used `unittest` for testing. Thats fine to start I guess... it has been ages that I did not code any python unittests (or any python actually).

remace07 commented

2022-12-14 14:38:38 +01:00

Author

Collaborator

for the commit "fix HLS", it comes from stable from which I did the branch. I think that's why it's still here. to be fair, i'm not a git wizard at all, I don't know why it appears if it's not normal.

to me, the "CLI parsing" is an independant concern on its own (needing a single class/object though). this is why there is a separate module, CliParser, with a class declaration(CLIParser), which is used only once in main (it replaces exactly what You
did before with

args = sys.args[2:]

but it will be easier to change prototypes later with this, as you mentionned in more recent issues, where You need more arguments/options.

what I did, is pretty much what you say (as much as I understand), but with another naming (I can rename files and variables if You prefer).

for the commit "fix HLS", it comes from stable from which I did the branch. I think that's why it's still here. to be fair, i'm not a git wizard at all, I don't know why it appears if it's not normal. to me, the "CLI parsing" is an independant concern on its own (needing a single class/object though). this is why there is a separate module, CliParser, with a class declaration(CLIParser), which is used only once in main (it replaces exactly what You did before with ```python args = sys.args[2:] ``` but it will be easier to change prototypes later with this, as you mentionned in more recent issues, where You need more arguments/options. what I did, is pretty much what you say (as much as I understand), but with another naming (I can rename files and variables if You prefer).

remace07 added 1 commit

2022-12-14 14:45:31 +01:00

🚚 rename modules and cli parser Class e42608f917

remace07 added 1 commit

2022-12-14 15:16:38 +01:00

📝 change readme doc on usage 2ea7808c0f

Barbagus commented

2022-12-14 15:32:24 +01:00

Collaborator

for the commit "fix HLS", it comes from stable from which I did the branch. I think that's why it's still here. to be fair, i'm not a git wizard at all, I don't know why it appears if it's not normal.

Yep, I look at the commit graph and it is a mistery to me :)

what I did, is pretty much what you say (as much as I understand), but with another naming (I can rename files and variables if You prefer).

Perfect ! Of course concerns can mean a lot of things. The names you used suggested to me that you made a module just for this class.

I guess what I mean by concerns is more scope than functionality:

interacting with ArteTV website -> www
interacting with API -> api
interacting with user -> cli
deal with HLS protocol -> hls
...

And the main() puts everything together.

That way if we implement some super fancy progress bar or elaborate AsciiArt output it can all go in cli. So far it was not really needed but I guess more stuff should now migrate from __main__ to cli.

> for the commit "fix HLS", it comes from stable from which I did the branch. I think that's why it's still here. to be fair, i'm not a git wizard at all, I don't know why it appears if it's not normal. Yep, I look at the commit graph and it is a mistery to me :) > what I did, is pretty much what you say (as much as I understand), but with another naming (I can rename files and variables if You prefer). Perfect ! Of course _concerns_ can mean a lot of things. The names you used suggested to me that you made a module just for this class. I guess what I mean by _concerns_ is more _scope_ than _functionality_: - interacting with ArteTV website -> `www` - interacting with API -> `api` - interacting with user -> `cli` - deal with HLS protocol -> `hls` - ... And the `main()` puts everything together. That way if we implement some super fancy progress bar or elaborate AsciiArt output it can all go in `cli`. So far it was not really needed but I guess more stuff should now migrate from `__main__` to `cli`.

remace07 added 1 commit

2022-12-14 15:38:27 +01:00

🚸 add documentation for user to arguments parser with -h flag df120e54d5

Barbagus commented

2022-12-15 12:02:32 +01:00

Collaborator

As for now, we always need the (url, version, resolution) arguments and we process them in that very order (and only the last one might ever have a default value one day, see below). So I don't necessarly see the point at puting them behind flags (-l, -r)

When #8 comes along, I can see who a --name-pattern (whatever the short version can be) could come to use. Because there should by (in my opinion) a default value for this, something like "{title}{separator}{subtitle}" with another --name-separator defaulting to " - " (or other)

Also, as you mentioned in #8 if there is ever a config file with default preferences for naming pattern, prefered resolution, having a --config-file to target the non default location (~/.delarte.toml for instance) makes sens to me.

For now I would keep the parser for later configurations, but keep the trio without flags.

What do you people think ?

As for now, we always need the (url, version, resolution) arguments and we process them in that very order (and only the last one might ever have a default value one day, see below). So I don't necessarly see the point at puting them behind flags (`-l`, `-r`) When #8 comes along, I can see who a `--name-pattern` (whatever the short version can be) could come to use. Because there should by (in my opinion) a _default_ value for this, something like "{title}{separator}{subtitle}" with another `--name-separator` defaulting to `" - "` (or other) Also, as you mentioned in #8 if there is ever a _config file_ with default preferences for naming pattern, prefered resolution, having a `--config-file` to target the non default location (`~/.delarte.toml` for instance) makes sens to me. For now I would keep the parser for later configurations, but keep the trio without flags. What do you people think ?

remace07 commented

2022-12-16 01:01:55 +01:00

Author

Collaborator

I think it is more "userproof" with flags: an user that provides a link, and a resolution but no language code would be badly interpreted by argparse: positionnal arguments HAVE TO be given in the very order of the prototype, because argparse creates a Namespace object, which is a key:value structure.

pathologic case:

If you define the prototype as

delarte url language resolution

if a user gives

delarte www.arte.tv 1080p

argparse library creates

Namespace(url="www.arte.tv", language="1080p", resolution=None)
# which will be given in args list in your script as:
["www.arte.tv","1080p",None]

it's up to You deciding whether you want to guide the users but annoy them more, or guide less but annoy them less.

btw, making this modification is easy, I already did it on my local branch, tell me if You want me pushing it...

I think it is more "userproof" with flags: an user that provides a link, and a resolution but no language code would be badly interpreted by argparse: positionnal arguments HAVE TO be given in the very order of the prototype, because argparse creates a Namespace object, which is a key:value structure. pathologic case: If you define the prototype as ```bash delarte url language resolution ``` if a user gives ```bash delarte www.arte.tv 1080p ``` argparse library creates ```python Namespace(url="www.arte.tv", language="1080p", resolution=None) # which will be given in args list in your script as: ["www.arte.tv","1080p",None] ``` it's up to You deciding whether you want to guide the users but annoy them more, or guide less but annoy them less. btw, making this modification is easy, I already did it on my local branch, tell me if You want me pushing it...

Barbagus referenced this pull request

2022-12-16 07:56:38 +01:00

CLI argument/options and default behaviours #1

Barbagus referenced this pull request

2022-12-16 08:14:38 +01:00

CLI argument/options and default behaviours #1

Barbagus commented

2022-12-16 08:29:35 +01:00

Collaborator

Alright, I discussed the named vs. positional in #1

For the rest it looks fine to me, last time I checked I picked up that

there was still some french docstrings
you rewrote the usage string in epilog, maybe you could put it as the module docstring and refer to it as __doc__ so it serves both as cli module documentation AND usage string from the parser (like it was done in __main__.py
you miss wrote the help string for -r (something about output directory)

Alright, I discussed the named vs. positional in #1 For the rest it looks fine to me, last time I checked I picked up that - there was still some french docstrings - you rewrote the usage string in `epilog`, maybe you could put it as the module _docstring_ and refer to it as `__doc__` so it serves both as `cli` module documentation AND _usage string_ from the parser (like it was done in `__main__.py` - you miss wrote the help string for `-r` (something about output directory)

remace07 added 5 commits

2022-12-16 10:42:46 +01:00

🚸 remove flags from script prototype 9736926074

🩹 naming: not "languages", "version"

🐛 💡 fixed bad help sentence for resolution argument 5bc0eeed97

💡 french translating docstrings 6d18007b24

📄 add licence comments top 62b7b22a62

📝 add more doc for CLI help string bff125ce7f

remace07 changed title from ~~WIP-CLI-argument-#1~~ to WIP: CLI-argument-#1

2022-12-16 12:26:59 +01:00

remace07 commented

2022-12-18 16:36:06 +01:00

Author

Collaborator

I am a bit confused why the the "Fix HLS..." commit shows as part of that branch. I am not familiar with Gitea and therfore I find it difficult to see the diff against stable without seeing all those changes as well. Maybe @freezed can share some git wizardry here :)

I tried to move all the changes introduced in this branch except the redundant one that creates conflicts in this branch in PR #18. it remains only a few files in conflict, and I don't know how to do this in gitea. in my local repo "git merge from stable branch" works automatically.

should I push my stable branch?

> I am a bit confused why the the "Fix HLS..." commit shows as part of that branch. I am not familiar with Gitea and therfore I find it difficult to see the diff against `stable` without seeing all those changes as well. Maybe @freezed can share some git wizardry here :) I tried to move all the changes introduced in this branch except the redundant one that creates conflicts in this branch in PR #18. it remains only a few files in conflict, and I don't know how to do this in gitea. in my local repo "git merge from stable branch" works automatically. should I push my stable branch?

Barbagus commented

2022-12-18 21:12:30 +01:00

Collaborator

Okay why not :)

remace07 changed title from ~~WIP: CLI-argument-#1~~ to CLI-argument-#1

2022-12-20 09:14:37 +01:00

remace07 changed title from ~~CLI-argument-#1~~ to WPI: CLI-argument-#1

2022-12-20 09:14:55 +01:00

Barbagus closed this pull request

2022-12-20 09:54:47 +01:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Rows
Columns

WPI: CLI-argument-#1 #17

Pull request closed