Billyzou0741326 / bilibili-live-monitor-ts

bilibili - b站直播监控
https://billyzou0741326.github.io/bilibili-live-monitor-ts/
MIT License
28 stars 4 forks source link

Make room collector strategy configurable #29

Closed blu3mania closed 4 years ago

blu3mania commented 4 years ago

Expiry in database; Dynamic rooms query interval, page size and sort type.

Also various changes:

Billyzou0741326 commented 4 years ago

git rebase -i HEAD~4 // squash 4 commits into 1

In the text editor that comes up, replace the words "pick" with "squash" next to the commits you want to squash into the commit before it. Save and close the editor, and git will combine the "squash"'ed commits with the one before it. Git will then give you the opportunity to change your commit message to something like, "Issue #100: Fixed retweet bug."

git push origin branch-name --force // must force push to resolve conflicts

git commit --amend // You can always edit your commit message

Full text available at https://github.com/todotxt/todo.txt-android/wiki/Squash-All-Commits-Related-to-a-Single-Issue-into-a-Single-Commit

Billyzou0741326 commented 4 years ago

Great, I'll review the changes once I find time. Thanks!

blu3mania commented 4 years ago

Thanks Billy. :)

Billyzou0741326 commented 4 years ago

I noticed some components are becoming tightly coupled in recent commits. For instance, RoomCollector and Database now requires configuration to be usable, meaning that we'll have to remember that we need to give the constructor a Strategy to create an object of those classes. This is likely an antipattern. Since we're not writing libraries (lol), we might want to minimize the number of configurations open to users. Note that for dynamicRoomsPageSize, we know the valid range and the best value for it, but the user can mess this up easily. I suggest leaving this out. The same if true for dynamicRoomsSortType.

In my next commit after merging, I plan to:

  1. Make the parameters for RoomCollector and Database optional
  2. Remove dynamicRoomsSortType and dynamicRoomsPageSize from the config

Are you ok with these?

blu3mania commented 4 years ago

I am fine with them. They are more for debugging purpose and I agree most won't even touch them.

Billyzou0741326 commented 4 years ago

Awesome

blu3mania commented 4 years ago

@Billyzou0741326 Just saw your new change and noticed that the calculation of expiry is in hours, not days.

Billyzou0741326 commented 4 years ago

Ouch