Closed Direwolfesp closed 3 months ago
Hey! Pretty neat addition 😄
I have some suggestions if you don't mind. One is a concern of mine, for the users that may already be using this script for scripting, this addition would disrupt how their setup works right now, what do you think about having this option as a toggle, a flag or something? The user who want to select a file or all of that would have to specify it first (maybe using an envar, so users wanting have this as their default would just set this envar somewhere)
Another suggestion, as we are at it already, why don't we implement a way of selecting more than just one file? We could use the user input space separated options as dictionary keys so there's no need to convert the input of the user to int, and if the specified key isn't tied to any file we just ignore that and accept all other valid options (taking care to display the invalid options selected), if there's no selection valid just asks the user to use something valid. (or something on that line of thought).
Though my last suggestion would raise some limitations as a really big list of files would not show entirely as some terminal emulators have a limited number of lines (as for that we could display only a batch of options and have a option (and or, pressing enter) to display more options).
What do you say?
Glad you liked the addition ^^
what do you think about having this option as a toggle, a flag or something? The user who want to select a file or all of that would have to specify it first (maybe using an envar, so users wanting have this as their default would just set this envar somewhere)
Sure, making it a envar or flag will prevent this being a breaking change and remain backward-compatible and it wont be very difficult to implement. Maybe a -i
/ --interactive
flag would do the job.
Another suggestion, as we are at it already, why don't we implement a way of selecting more than just one file? We could use the user input space separated options as dictionary keys so there's no need to convert the input of the user to int,
Yeah, after adding the function i was thinking of making it so you can pick more than one by inputing several numbers for example $: 1 3 4
. So, your idea is to create a new dictionary so each key is mapped to a filename ie: {"1":"file.txt", "2": "file2.mp4"}
? That means we need to create a new dict? Feel free to explain it with more detail as im kinda new to this and maybe i didn't get it XD.
my last suggestion would raise some limitations as a really big list of files would not show entirely as some terminal emulators have a limited number of lines
Hmm, considering this problems maybe is worth looking into the built in library called curses, it allows to create scrollabe list with multiple selections. Here is a quick example from GPT. Thought this may require more refactoring and time in my opinion, but it would look cool! Maybe we can just push the flag or envar thingy and then work in the rest stuff in a new branch idk.
Anyway, im totally open to keep making some small contributions as i just started python yesterday by reading your script xd, (i causually do some c++ instead).
Sure, making it a envar or flag will prevent this being a breaking change and remain backward-compatible and it wont be very difficult to implement. Maybe a -i / --interactive flag would do the job.
I like the idea of having an envar better (like a GF_INTERACTIVE), the flags would work better if we were already using flags to start with, which isn't the case (at least not yet 😅) so to keep it simple it's expected to the first argument be the url and the second be the password.
Yeah, after adding the function i was thinking of making it so you can pick more than one by inputing several numbers for example $: 1 3 4. So, your idea is to create a new dictionary so each key is mapped to a filename ie: {"1":"file.txt", "2": "file2.mp4"} ? That means we need to create a new dict? Feel free to explain it with more detail as im kinda new to this and maybe i didn't get it XD.
Hmm, considering this problems maybe is worth looking into the built in library called curses, it allows to create scrollabe list with multiple selections. Here is a quick example from GPT. Thought this may require more refactoring and time in my opinion, but it would look cool! Maybe we can just push the flag or envar thingy and then work in the rest stuff in a new branch idk.
Yeah, if you want to keep simple, splitting the user input and using them as keys for the files, just exactly as the list index without the need to convert them to integers and raising exceptions, the .get("keyword") is more straight forward than handling the out of index and or value error exception.
Or you could go with the curses route, I'm also ok with that 😃 whatever is better for you (this ties even better with the idea of interactively selecting which files to download).
Just add the commits to this PR and we work it from there.
@Direwolfesp actually curses is only available on unix, meaning that it isn't available on Windows https://docs.python.org/3/howto/curses.html. So I think it's better not use it and stick with the initial idea of just show "N" items to be chosen from and add a option to show "N" more items to be chosen.
Aight, ill pass on the curses thing. One question thought, regarding the multiple selection, is it necessary to pass self._threadedDownloads(content_dir, files_link_list)
the same file_link_list object (as it is rn) or can i create a new list[dict[str, str]] with the selected files and pass it to the function?
Okai, I ve added in the last commit the envar and the multiple files. Although it does the trick im not sure this is the way you intended to be implemented xd.
@Direwolfesp sorry about all of that, I was thinking of a way to make your implementation easier to you.
Have a look at https://github.com/ltsdw/gofile-downloader/commit/b075cb764213fa5509613af8efefb26c640f54c8
Now that we have:
You can simply use that. It works by setting an identifier/index as keyword for each file (but not for directories), where the top most file have the largest identifier (bottom up because of recursion).
Oh i see, that member variable makes it more easy now. Ill try to rewrite it now.
Nice 😃 , could you please squash the 4 commits into one?
Do i do that now or after updating my thing? Also, how can i update my branch so that is has your latest changes from main? (sorry im not used to use git very much)
As there's no conflict right now, your branch and mine have the same changes already.
Also, use:
interactive: str | None = getenv("GF_INTERACTIVE")
for simplicity and to keep consistency with the variable naming.
Also add an entry the README explaining the usage of the GF_INTERACTIVE (setting it to 1 to use it).
You can also simplify:
- user_input: str = input("Enter the index of the files to download separated with spaces : ")
- input_list: list[str] = user_input.split(" ")
+ input_list: list[str] = input("Enter the index of the files to download separated with spaces : ").split()
You can also simplify the filtering logic to something like:
filtered_files: dict[str, str] = {k: v for k, v in self._files_info.items() if k in input_list}
So basically do the changes and squash the commits into one 😃
Aight, i instead just merged your branch with mine, and will add my changes again with the corrections. One question is: if i do
interactive: str | None = getenv("GF_INTERACTIVE") for simplicity
wouldnt it be weird to the declare the envar as an int like GF_INTERACTIVE=1
and still work?
Aight, i instead just merged your branch with mine, and will add my changes again with the corrections. One question is: if i do
interactive: str | None = getenv("GF_INTERACTIVE")
wouldnt it be weird to the declare the envar as an int likeGF_INTERACTIVE=1
and still work?
No, it's kind of convention that shells (cmd or powershell included) treat envars as strings by default, also I find it more sane to use 1 than "True" or any other value, when checking we would have to check explicitly for the string "1"
if interactive == "1":
Any other value should be ignored.
Or better yet:
interactive: bool = getenv("GF_INTERACTIVE") == "1"
if interactive:
...
There is another consideration, the previuos method by having two containers, one with all the files and the filtered one, you were able to for example input:
$: 1 4 2 0
and if it detects a 0 it will just use the full file_list.
I think is better to remove that possibility and only work with the listFiles and not add the DOWNLOAD index. What do u think?
ISSUE: I think there is an error with the actual script on main branch. Its trying to cd into an inexistent directory.
Starting, please wait...
Traceback (most recent call last):
File "/home/dire/deleteme.py", line 444, in <module>
Main(url=url, password=password)
File "/home/dire/deleteme.py", line 72, in __init__
self._parseUrlOrFile(url, password)
File "/home/dire/deleteme.py", line 397, in _parseUrlOrFile
self._download(url_or_file, _password)
File "/home/dire/deleteme.py", line 376, in _download
if not listdir(self._content_dir) or not self._files_info:
^^^^^^^^^^^^^^^^^^^^^^^^^^
FileNotFoundError: [Errno 2] No such file or directory: '/home/dire/NOvtFE'
Sorry about that 😅 should be fixed by https://github.com/ltsdw/gofile-downloader/commit/6381c4ffeebeed451436feeaf54c41e157041b35
There is another consideration, the previous method by having two containers, one with all the files and the filtered one,
you were able to for example input: $: 1 4 2 0 and if it detects a 0 it will just use the full file_list.
I think is better to remove that possibility and only work with the listFiles and not add the DOWNLOAD index. What do u think?
Have the empty string be the default for downloading all the files (the user just pressed enter, without typing anything).
The keys are 1 indexed so there's the 0 key spare (I don't see any clear use for it).
If the user types 1 2 nonsense 0 (only 1 and 2 will be valid keys the other two will just be ignored).
If no valid input were to be found, the program will just exit or ask for something valid.
Also we have to have an option to display more items to download (m for more, or something like that)
I think showing a list of 50 items is ok, I can't think of any terminal emulator with such a low number of limit lines.
Also I think is more intuitive to present the list of items first and then ask for the user input.
Also I think it's better to display the list of options as a big string, so it will be easy to use the carrier character to clear the screen and display other items or when ask for the input again.
Okai i already commited some changes using the new dict, and following this:
Have the empty string be the default for downloading all the files (the user just pressed enter, without typing anything).
and
If the user types 1 2 nonsense 0 (only 1 and 2 will be valid keys the other two will just be ignored).
however,
If no valid input were to be found, the program will just exit or ask for something valid.
in the actual code, no valid input == "" (empty string/ENTER) so it will download all.
Thank you, could you squash these 4 commits into one please?
I think they were squashed correctly now
lgtm 😄
Thank you for this great addition.
Hi, i just added some simple function to visualize the content of
files_link_list
and select a single item to download (or all of them). It may not be up to the repo standards but it is handy most of the sometimes.