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

fix: allow using --select-nth with --continue #1366

Merged
merged 4 commits into from
Jun 14, 2024

Conversation

FireEgl
Copy link
Contributor

@FireEgl FireEgl commented Jun 10, 2024

Allows using --select-nth with --continue to select the anime when there's multiple to choose from.

Pull Request Template

Type of change

  • Bug fix
  • Feature
  • Documentation update

Description

This adds a check for --select-nth when using --continue to select the anime, so it can be done non-interactively.
Updated to work with the current code base. =)

Checklist

  • any anime playing
  • bumped version

  • next, prev and replay work
  • -c history and continue work
  • -d downloads work
  • -s syncplay works
  • -q quality works
  • -v vlc works
  • -e select episode works
  • -S select index works
  • -r range selection works
  • --skip ani-skip works
  • --skip-title ani-skip title argument works
  • --no-detach no detach works
  • --dub and regular (sub) mode both work
  • all providers return links (not necessarily on a single anime, use debug mode to confirm)

  • -h help info is up to date
  • Readme is up to date
  • Man page is up to date

Additional Testcases

  • The safe bet: One Piece
  • Episode 0: Saenai Heroine no Sodatekata ♭
  • Unicode: Saenai Heroine no Sodatekata ♭
  • Non-whole episodes: Tensei shitara slime datta ken (ep. 24.5, ep. 24.9)

Allows using --select-nth with --continue to select the anime when there's multiple to choose from.
@FireEgl FireEgl requested a review from Derisis13 as a code owner June 10, 2024 13:13
@71zenith
Copy link
Collaborator

as i said before, u cant even predict which index will have which anime, making this useless

@FireEgl
Copy link
Contributor Author

FireEgl commented Jun 10, 2024

as i said before, u cant even predict which index will have which anime, making this useless

Yes, it's unpredictable which anime it downloads/plays. But it also doesn't matter. I just want to periodically run the script to have all my animes downloaded (ani-cli -d -c -S 1). I get that it kind of mis-uses what --select-nth was meant for, but it works.
The alternative is to rework a lot of the script to have a properly working non-interactive mode.. But since this might be the only use-case, I don't see the problem with a little bit of a hack.

@port19x
Copy link
Collaborator

port19x commented Jun 11, 2024

Valid use case for a tiny change

@port19x port19x changed the title Update ani-cli to allow using --select-nth with --continue fix: allow using --select-nth with --continue Jun 11, 2024
Copy link
Collaborator

@port19x port19x left a comment

Choose a reason for hiding this comment

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

I wonder how this code doesn't trigger SC2015

@FireEgl
Copy link
Contributor Author

FireEgl commented Jun 11, 2024

I wonder how this code doesn't trigger SC2015

I tried checking the same code without the id= part and shellcheck gives a SC2015 (info) on it..
So, if somehow the interactive prompt fails, it's going to try to set it using --select-nth which will also fail, and the next line is a [ -z "$id" ] && exit 1 anyway, so it's fine. =)

ani-cli Outdated
@@ -427,7 +427,8 @@ case "$search" in
anime_list=$(while read -r ep_no id title; do process_hist_entry & done <"$histfile")
wait
[ -z "$anime_list" ] && die "No unwatched series in history!"
id=$(printf "%s" "$anime_list" | nl -w 2 | sed 's/^[[:space:]]//' | nth "Select anime: " | cut -f1)
# Prompts for the anime, or selects based on the index (--select-nth) value if provided:
[ -z "${index##*[!0-9]*}" ] && id=$(printf "%s" "$anime_list" | nl -w 2 | sed 's/^[[:space:]]//' | nth "Select anime: " | cut -f1) || id=$(printf "%s" "$anime_list" | sed -n "${index}p" | cut -f1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would have expected the logic to go like this, but maybe I'm missing something

Suggested change
[ -z "${index##*[!0-9]*}" ] && id=$(printf "%s" "$anime_list" | nl -w 2 | sed 's/^[[:space:]]//' | nth "Select anime: " | cut -f1) || id=$(printf "%s" "$anime_list" | sed -n "${index}p" | cut -f1)
[ -z "${index##*[!0-9]*}" ] && id=$(printf "%s" "$anime_list" | nl -w 2 | sed 's/^[[:space:]]//' | nth "Select anime: " | cut -f1)
[ -z "${index##*[!0-9]*}" ] || id=$(printf "%s" "$anime_list" | sed -n "${index}p" | cut -f1)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also prefer port's approach. would be better off being verbose

@71zenith
Copy link
Collaborator

71zenith commented Jun 11, 2024

${index##*[!0-9]*} very cool way to type check and it seems this is posix as well.
might want to incorporate more into the codebase rather than -eq which is far more ambiguous in what its trying to achieve

@71zenith 71zenith self-requested a review June 11, 2024 13:14
@@ -1,6 +1,6 @@
#!/bin/sh

version_number="4.8.9"
version_number="4.8.10"
Copy link
Collaborator

Choose a reason for hiding this comment

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

bump it to 4.9.0

ani-cli Outdated
@@ -427,7 +427,8 @@ case "$search" in
anime_list=$(while read -r ep_no id title; do process_hist_entry & done <"$histfile")
wait
[ -z "$anime_list" ] && die "No unwatched series in history!"
id=$(printf "%s" "$anime_list" | nl -w 2 | sed 's/^[[:space:]]//' | nth "Select anime: " | cut -f1)
# Prompts for the anime, or selects based on the index (--select-nth) value if provided:
[ -z "${index##*[!0-9]*}" ] && id=$(printf "%s" "$anime_list" | nl -w 2 | sed 's/^[[:space:]]//' | nth "Select anime: " | cut -f1) || id=$(printf "%s" "$anime_list" | sed -n "${index}p" | cut -f1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also prefer port's approach. would be better off being verbose

ani-cli Outdated
@@ -427,7 +427,8 @@ case "$search" in
anime_list=$(while read -r ep_no id title; do process_hist_entry & done <"$histfile")
wait
[ -z "$anime_list" ] && die "No unwatched series in history!"
id=$(printf "%s" "$anime_list" | nl -w 2 | sed 's/^[[:space:]]//' | nth "Select anime: " | cut -f1)
# Prompts for the anime, or selects based on the index (--select-nth) value if provided:
Copy link
Collaborator

Choose a reason for hiding this comment

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

also u can remove the comment. not really necessary with the context

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dunno how to do this without making another branch and pull-request like you guys, but what about this:

--- ani-cli	2024-06-10 07:55:18.846673971 -0500
+++ ani-cli.fireegl-2	2024-06-11 08:21:47.636001449 -0500
@@ -427,7 +427,10 @@
         anime_list=$(while read -r ep_no id title; do process_hist_entry & done <"$histfile")
         wait
         [ -z "$anime_list" ] && die "No unwatched series in history!"
-        id=$(printf "%s" "$anime_list" | nl -w 2 | sed 's/^[[:space:]]//' | nth "Select anime: " | cut -f1)
+        case "$index" in
+           ''|*[!0-9]*) id=$(printf "%s" "$anime_list" | nl -w 2 | sed 's/^[[:space:]]//' | nth "Select anime: " | cut -f1) ;;
+           *) id=$(printf "%s" "$anime_list" | sed -n "${index}p" | cut -f1) ;;
+        esac
         [ -z "$id" ] && exit 1
         title=$(printf "%s" "$anime_list" | grep "$id" | cut -f2 | sed 's/ - episode.*//')
         ep_list=$(episodes_list "$id")

Copy link
Collaborator

Choose a reason for hiding this comment

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

dont use case. to maintain parity with rest of the codebase, just check it twice like port did. not very efficient but meh who cares.

Copy link
Collaborator

Choose a reason for hiding this comment

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

just pull the changes ? i assume u havent pulled the version bump port did.
and make a new commit

Copy link
Collaborator

Choose a reason for hiding this comment

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

dont open a new pr please

Copy link
Collaborator

Choose a reason for hiding this comment

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

if u cannot do this, i can make the changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please do. =)

Copy link
Collaborator

Choose a reason for hiding this comment

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

done, if ure having issues u can just use the github editor line . just open the file on gh and use the pen button to edit there

Copy link
Collaborator

Choose a reason for hiding this comment

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

port can handle the rest

@port19x
Copy link
Collaborator

port19x commented Jun 12, 2024

@71zenith you sure we wanna bump to 4.9?

@port19x
Copy link
Collaborator

port19x commented Jun 12, 2024

We do have countdown to next episode as a feature I guess

@71zenith
Copy link
Collaborator

i believe we have enough fixes accumulated? did we aim at having some milestones for each release?
just thought that .10 felt wrong. if u want we can wait a bit for the logger feature

@port19x
Copy link
Collaborator

port19x commented Jun 12, 2024

Let's wait for the logger feature then.
Almost all our downstreams being up-to-date is just very satisfying to look at on repology, so I'm hesitant.
But you're right, we have a lot of small bumps and features

@71zenith 71zenith mentioned this pull request Jun 12, 2024
@port19x port19x requested a review from 71zenith June 12, 2024 12:19
@port19x
Copy link
Collaborator

port19x commented Jun 14, 2024

@71zenith you're gonna approve or no?

@71zenith
Copy link
Collaborator

approved, sorry i was at school

@71zenith 71zenith merged commit e4da399 into pystardust:master Jun 14, 2024
7 checks passed
@KuraiZ0
Copy link

KuraiZ0 commented Nov 29, 2024

I like your work !
I havé a suggestions if possible, add others languages like French, Spain Will give you More visibility !

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.

4 participants