Skip to content

feat: improved tracking system #70

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

Draft
wants to merge 92 commits into
base: main
Choose a base branch
from
Draft

feat: improved tracking system #70

wants to merge 92 commits into from

Conversation

flloschy
Copy link
Contributor

Rework of the whole tracking system
Featuring new commands and way better performance
(and commenting whoaaaaaaaa)

Copy link
Member

@StrangeGirlMurph StrangeGirlMurph left a comment

Choose a reason for hiding this comment

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

Haven't looked at the code yet. Just used the 10min I had to comment on the first things that came up.

Copy link
Member

Choose a reason for hiding this comment

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

Really not necessary. Just let the interface in the config file. Makes it way easier to quickly look it up and read the docs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The doc strings (/** */) are there so the IDE can show the docs when hovering over the value. Just cleans up the config files so there is less confusion in case less experienced users try Jambo. And you can just jump to the interface with ctrl click anyway

Copy link
Member

Choose a reason for hiding this comment

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

That's a VSCode thing... It should be just a scroll away. One shouldn't need a high end IDE to figure things like that out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats not just a VSC thing? Such doc strings are part of js/ts and I do believe that no "high end IDE" is needed to see them when hovering over it

Copy link
Contributor

Choose a reason for hiding this comment

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

Not entirely part of JS, but an extension called jsdocs. VScode is not the only ide that supoorts it (Webstorm supoorts it). It's also a human readable format so anyone reading this file can see all this info, if their IDE doesn't pick it up.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah sure "That's a VSCode thing" just meant that it's an IDE thing.

It's also a human readable format so anyone reading this file can see all this info, if their IDE doesn't pick it up.

Yeah sure but that's exactly my point. People shouldn't have to scroll to the top see where its imported from and then go to that file just to look up something that trivial which really doesn't hurt to be in that same file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively we could just
export const config = {...} as const;
without having any interfaces anywhere

Copy link
Member

Choose a reason for hiding this comment

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

I think having a separate interface is good. "Stops" people from just blindly messing up stuff.

Copy link
Contributor Author

@flloschy flloschy left a comment

Choose a reason for hiding this comment

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

Resolving again...

src/db.ts Outdated
/** id of the log */
id: string;
/** When a log got logged */
date: Date;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No? Why?
Just a date object, the serializer takes care of converting everything to iso date and the reserialize returns a new date object using the iso string stored in the db

/** How long a User played a certain game
* (Or the other way around)
*/
playtime: number;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

idk, I simply dont like duration
And ms is what a presence-update gives you anyway
Sso why convert to duration, then instantly sterilize to ms again just to desterilize again to perform a simple math or timestamp thing...
Im not seeing the benefits behind for changing it

@StrangeGirlMurph
Copy link
Member

StrangeGirlMurph commented Jun 9, 2023

All

  • Remove seconds and ms from everywhere
  • always explain what the default values for optional values are
  • don't separate the date and time wherever you did. i.e. those are two separate time stamps
    image
  • link the users and don't just use their nicknames

/tracker game

general statistics

  • remove latest logs
  • replace "Most logged users" with "Most logs"
  • replace "User with most playtime" with "Most playtime"
  • switch the order of "Most logs" and "Most playtime"

top 5 most played games

  • can be removed that info is already available in general statistics

top 5 most logged games

  • can be removed that info is already available in general statistics

latest 5 logs

  • remove the command and just add it as an option to "/tracker latest" and call it "/tracker logs" for example and call the remaining general statistics one "tracker game-stats" for example
  • show more then 5 logs

/tracker statistics

  • again switch most played and most logged
  • Make these four one field "Total ...: \n playtime: ... \n logs: ..." etc.
    image

/tracker user

default

  • Don't waste so much space! Put the total things in a single field
    image

game

  • this doesn't make sense the selected game is minecraft and I just get the default
    image
  • the latest logs are in reverse

@Chicken Chicken changed the title New tracking system feat: improved tracking system Jun 9, 2023
@StrangeGirlMurph StrangeGirlMurph marked this pull request as draft October 20, 2023 18:48
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