Skip to content
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

Pics Cache Enhancements #409

Open
wants to merge 32 commits into
base: master
Choose a base branch
from

Conversation

Revadike
Copy link
Contributor

Original: #389

@Revadike
Copy link
Contributor Author

Revadike commented Sep 1, 2024

@DoctorMcKay when are you finally implementing this feature? Do I have to keep maintaining and using my own fork? :(

@DoctorMcKay
Copy link
Owner

Could you resolve conflicts please? I'll merge it this week once conflicts are resolved.

@Revadike
Copy link
Contributor Author

Revadike commented Sep 3, 2024

Could you resolve conflicts please? I'll merge it this week once conflicts are resolved.

Done. Still needs a lint fix though. Also, I have not tested the new merged changes. So, I recommend testing them. Perhaps some tweaks and refactor could be good as well. The time I've been using this fork, I had no issues, except for the occasional "picscache not found for sub xxx" warnings, which I'm not sure why that happens.

* @param {boolean} [inclTokens=false] - If true, automatically retrieve access tokens if needed
* @param {function} [callback]
* @param {boolean} inclTokens - If true, automatically retrieve access tokens if needed
* @param {function} callback
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, unwanted change.

// that things work regardless of whether there's a NUL byte at the end, just remove it if it's there.
appInfoVdf = appInfoVdf.replace(/\0$/, '');
let appinfo = null;
if (app.buffer) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe this is why sometimes pics cache is missing? hmm

Comment on lines +509 to +519
// console.log(
// "requestType:", requestType,
// "app request:", appids.length,
// "pkg request:", packageids.length,
// "app unknown:", response.unknownApps.length,
// "pkg unknown:", response.unknownPackages.length,
// "app cache:", Object.keys(cached.apps).length,
// "pkg cache:", Object.keys(cached.packages).length,
// "app to refresh:", apps.length,
// "pkg to refresh:", packages.length,
// );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is really useful for debugging, but you may want to remove it.

}

if (this.options.picsCacheAll && this.options.savePicsCache) {
this._warn('Both picsCacheAll and savePicsCache are enabled. Unless a custom storage engine is used, beware that this will cause a lot of disk IO and space usage!');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe remove this warning? I have been seeing this every time I start my bot.

@Revadike
Copy link
Contributor Author

Revadike commented Sep 3, 2024

Btw, may be worth checking our past discussion: #389

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants