-
Notifications
You must be signed in to change notification settings - Fork 2
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Co-authored-by: Murphy Sünnenwold <[email protected]>
There was a problem hiding this 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; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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
Rework of the whole tracking system
Featuring new commands and way better performance
(and commenting whoaaaaaaaa)