Rotating log file for #1534

[Kiduzk]

An implementation for #1534

[nuki-chan[bot]]

Lines 6-15

Ohayou, esteemed developer-san! Nuki has detected your intentions to improve logging, and while the effort is commendable, there are a few nuances that could make this even better! Let's tighten up those logs like we're prepping for the grand anime convention! 🌠

• The pad function is super adorable, doing its own little "I must include leading zeros" dance, but what if I told you we could avoid the dance altogether? JavaScript's String.prototype.padStart is like the cool senpai that's been around the block and has all the answers. Let's use that instead, and Nuki promises you'll still keep the cuteness factor! 🎀
• The check if (!time) is giving Nuki déjà vu vibes because it looks like we're preparing for some time travel! But since we're not in a sci-fi episode, perhaps we should ensure consistency in naming patterns between log files during no-time times and regular-time times, ne? 🚀
• Now, about that nested path building with month folders, Nuki wonders, do we have the code elsewhere to handle the creation of these directories? They don't just pop into existence like a magical girl transformation, after all! Make sure there's logic to handle directory creation if we're segmenting logs this way. ✨

Here's a little code magic from Nuki to help out:

const logNameGenerator = (time, index) => {
  if (!time) return path.join(app.getPath('userData'), 'logs', 'nuclear-error.log');

  const month = time.getFullYear().toString() + (time.getMonth() + 1).toString().padStart(2, '0');
  const day = time.getDate().toString().padStart(2, '0');
  const hour = time.getHours().toString().padStart(2, '0');
  const minute = time.getMinutes().toString().padStart(2, '0');

  return path.join(app.getPath('userData'), 'logs', `${month}/${month}${day}-${hour}${minute}-${index}-nuclear-error.log`);
};

Lines 16-25

Lookie here! We're getting all fancy with rotating log files! 📜 But, there's always room for a little more polish, isn't there? Here are some hints from your code-review kouhai:

• The log rotation size has shrunk from a bountiful '5M' to a mere '1M'. While Nuki admires minimalism, do ensure: "Does this size make sense for our use-case?" Smaller can be cuter, but it should also be practical. 🌀
• The maxFiles property mysteriously appeared like a secret character being introduced late into the season. It's indeed a good practice to limit the number of log files! Good job on that, but let's document why we chose 3 as the magic number, okay? 🧙‍♀️
• Lastly, the compression method 'gzip' will indeed make our logs slimmer, much like a good diet before cosplaying. Yet, I hope we're not compressing prematurely—consider the performance impact and the frequency of access before deciding to compress logs.

Remember, following Nuki's advice is like reading manga -- it's not required but it often leads to enlightenment, and occasionally, a good laugh. 😜 Also, Nuki might make a mistake sometimes, because even bots powered by the latest AI technologies can have bugs (but don't tell anyone, okay?). 🙊

Now, go forth and make these logs spectacular! Ganbatte, developer-chan! 💖✨

[nukeop]

Thanks for contributing. Let's discuss a couple of details in the comments

[nukeop]

Because you force-pushed to your branch, I can't push to your branch anymore, because the history is messed up and it's no longer continuous. Not sure how to add the tests I wrote to this PR.

[Kiduzk]

Hey @nukeop I apologize for the hassle I created with the force push. Is there a way we can fix it? Please let me know if you need me to do anything regarding the force push issue or the testing, I am more than willing to help. I would really appreciate your guidance if possible, I am kind of new to opensource stuff

[nukeop]

I tried to do several things and I have no idea how to fix this. I tried pushing to a parallel branch: https://github.com/nukeop/nuclear/tree/Kiduzk-temp But then I can't merge this branch into your master. Try it yourself, maybe you'll be able to.

In general you should never force push, especially to a branch others might be working on, it destroys git history. It's only allowable in cases where you commit important secrets that can't be rotated (even in those cases it's too late to protect these secrets anyway).

[nukeop]

Btw, the test on that branch is unfinished, and I'm not convinced it's going to be necessary. Maybe I could just merge this PR?

[Kiduzk]

As far as the testing goes, merging works with me. I run some manual tests with small log sizes and the output seems to be as we want it. This is one screenshot I have

[Kiduzk]

Also, I was able to merge your branch with my master branch locally. Would you suggest that I push these changes to my GitHub fork associated with this PR?

[nukeop]

Yep, push them to this branch. If you do, I'll be able to add further commits

[Kiduzk]

Sounds good, I have pushed them

[nukeop]

Now that I pulled this locally, it seems to be fixed. Thanks.

[nukeop]

For some reason I still can't push to your branch. Could you please remove the last test in logger.test.ts and leave everything else? And then we could merge this PR.

[Kiduzk]

Sure, no problem. I have removed the last test, let me know if that works

[nukeop]

Can you remove the last test in that file? You only deleted some mocks. I wanted to do it myself but I still can't push to your branch.

[Kiduzk]

Yea, no problem. How about now

[nukeop]

Hey, sorry I did not merge this yet, but I - literally - cannot:

I tried resolving these conflicts but again, I couldn't pull the changes locally, so I'm stuck. I think a good course of action would be to simply create a brand new branch from master, and manually copy these changes. Then I could merge that branch, and adapt it later manually when it's merged to master.

[Kiduzk]

I guess I learnt lesson to not force push for future, thanks or bearing with me. I can get started on copying to new branch