search

ShokoAnime / ShokoServer

Repository for Shoko Server.
http://shokoanime.com/shoko-server/
MIT License
389 stars 75 forks source link

feature: expose shoko events through SignalR + more #991

Closed revam closed 2 years ago

revam commented 2 years ago

expose the Shoko events through SignalR and refactor the plugin file events.

revam commented 2 years ago

The file events are tested on my machine:tm: image image

Cazzar commented 2 years ago

Just a noticed pattern - it seems very often you seem to be adding extra changes unrelated to what you are doing. For example

  1. adding the docblocks to random objects, while it's not horrible of an idea - it makes it more complex to review.
  2. Why are you adding the : EventArgs to some of the already existing event args.

If these are done for good reason, it's usually worth noting into the PR description

revam commented 2 years ago
  1. I didn't add any doc-blocks to any "random" objects in this PR — every added doc-block is followed by an added or updated field. I will reverse back the order of the fields in FileEventArgs though, so it's easier to see what actually changed.
  2. Because to me is an *EventArgs not extending System.EventArgs (or a derived class) the same as an *Exception not extending System.Exception. Both patterns work in practice, but personally I don't like it.

Will try to write more details in the description next time.

revam commented 2 years ago

Fixed up the order of the existing fields, removed some empty lines at the beginning of the new files, and removed unused dependencies/imports in the new files.

Cazzar commented 2 years ago
  1. I didn't add any doc-blocks to any "random" objects in this PR — every added doc-block is followed by an added or updated field. I will reverse back the order of the fields in FileEventArgs though, so it's easier to see what actually changed.
  2. Because to me is an *EventArgs not extending System.EventArgs (or a derived class) the same as an *Exception not extending System.Exception. Both patterns work in practice, but personally I don't like it.

Will try to write more details in the description next time.

For future reference, I would suggest discussing changes you want to make "because you don't like it" because there may be unknown side-effects, or reasons it has been done explicitly.

The reason I said random, is when looking at the issue itself, and documentation you had included, a lot of it seemed unrelated, to the scope of "Expose x events"

revam commented 2 years ago

Further to this, when making any changes to Shoko.Plugin.Abstractions, it will force any person who uses daily and a custom plugin to rebuild it, because the dependency changes, so I would suggest only making necessary changes, or batching them together.

  1. I just batched them togther.
  2. I'm using my plugin built for 4.1.1 and it's working fine — even on the daily built earlier today — because I didn't touch any of the renamer events, only the new (and hopefully still unused) events introduced in (I think it was) 4.1.2.
Cazzar commented 2 years ago

Further to this, when making any changes to Shoko.Plugin.Abstractions, it will force any person who uses daily and a custom plugin to rebuild it, because the dependency changes, so I would suggest only making necessary changes, or batching them together.

  1. I just batched them togther.
  2. I'm using my plugin built for 4.1.1 and it's working fine — even on the daily built earlier today — because I didn't touch any of the renamer events, only the new (and hopefully still unused) events introduced in (I think it was) 4.1.2.
  1. I have seen this happen too many times before, I was just mentioning in terms of previous experience