justchokingaround / jerry

watch anime with automatic anilist syncing and other cool stuff
GNU General Public License v3.0
291 stars 20 forks source link

added feature: ask user for score after completing a show. #23

Closed Ar4m1s closed 1 year ago

Ar4m1s commented 1 year ago

I made the changes necessary for issue #22. Variable names and such likely need to be changed.

justchokingaround commented 1 year ago

thanks a lot for the effort you've put into this PR, i really appreciate it. i understand that shellscripting can be a bit hard when you start off, so if you have any questions, just ask me, i'll be glad to help. there is a lot of feedback, if you find something hard or too confusing, i can implement it myself too, that is not a problem for me

Ar4m1s commented 1 year ago

What is the way forward then? I totally understand if this feature will not be added, or at least not how I implemented it. I just want to know if or when this will be implemented.

justchokingaround commented 1 year ago

this feature is good, i approve of it, i only want it be used as a config option though, not as an argument passed directly to the script. it'll be merged when all of the feedback is fixed. i meant that i can help with implementing some of the things i mention in the feedback, that you might find hard/confusing. does that make sense?

Ar4m1s commented 1 year ago

If I understand it correctly, you are willing to help with making the pull request ready to be merged. And yes I understand that you do not want to add it as an argument. I will simply have to remove it from the commit then. Is that correct?

justchokingaround commented 1 year ago

yep, just remove the lines that deal with command line arguments in your fork, and implement the feedback

Ar4m1s commented 1 year ago

I have not been able to test with manga though. Should support for manga be added anyway?

justchokingaround commented 1 year ago

just leave the manga implementation as is, i'll test and fix the manga part when the rest is implemented, don't worry about it for now

Ar4m1s commented 1 year ago

I removed the code for command line arguments and amended the commit.

justchokingaround commented 1 year ago

great! can you implement the feedback now?

Ar4m1s commented 1 year ago

Feedback as in feedback you haven't given yet? Then yes.

justchokingaround commented 1 year ago

no, i mean the feedback i already gave: image

Ar4m1s commented 1 year ago

Oh sorry, where is that feedback? I am new to this.

justchokingaround commented 1 year ago

np! you can find it here: https://github.com/justchokingaround/jerry/pull/23/files

justchokingaround commented 1 year ago

when you click on a PR, you can find it when you click on Files changed

Ar4m1s commented 1 year ago

It feels like there is something obvious I am missing. when I click Files changed I just see the diff file. No comments.

justchokingaround commented 1 year ago

you're right, i forgot to publish the review, sorry about that :skull:

Ar4m1s commented 1 year ago

Not relevant to the code, but is there a lsp I can use? I am using neovim.

justchokingaround commented 1 year ago

yes, i use bashls: image in Mason: image

i also recommend u use shellcheck for your linter and shfmt as your formatter

justchokingaround commented 1 year ago

https://github.com/justchokingaround/dotfiles/blob/main/coding/neovim/nvim/lua/plugins/coding.lua#L162-L208 this is probably relevant; you can ignore everything other than the stuff i mentionned in the previous comment

Ar4m1s commented 1 year ago

Since I don't use mason I just installed bashls shellfmt and shellcheck and added the neovim config for bashls. Thank you, this will save me a lot of pain.

Ar4m1s commented 1 year ago

When i use shellfmt or use tab in neovim it uses tabs with 8 character width. How could I change this to 4 to avoid changing the formatting?

justchokingaround commented 1 year ago

this is what my null-ls configuration does: https://github.com/justchokingaround/dotfiles/blob/main/coding/neovim/nvim/lua/plugins/coding.lua#L192C38-L208

Ar4m1s commented 1 year ago

That did not change anything for me for some reason.

Ar4m1s commented 1 year ago

When i skip the rest of the episode with > the progress does not update, is this intentional?

justchokingaround commented 1 year ago

yes, since there is not much i can do about it

Ar4m1s commented 1 year ago

I removed the immediate_score function and added the functionality in the update_score using an extra parameter

justchokingaround commented 1 year ago

thanks for your PR!