Protected / rowboat

A fully modular, multi-environment chat bot.
Apache License 2.0
4 stars 2 forks source link

Improved json saving, added archiving for overwrites, added some test… #2

Closed mkumpan closed 4 years ago

mkumpan commented 6 years ago

… coverage.

mkumpan commented 6 years ago

@Protected, good call, I just noticed myself. I should be able to rebase this.

mkumpan commented 6 years ago

@Protected, @AWRyder, looks like our installation is based off the modeveroles branch. And that has diverged with dev by quite a bit. What's the best strategy here? Can we merge them somehow?

Protected commented 6 years ago

What are your needs? Are you an acquaintance of AWRyder's? Do you specifically need ModEveRoles, or is there other stuff you need more? You could just stick with that version if you mainly need ModEveRoles.

Ideally, ModAPI should be rewritten to use express, ModEveRoles should be rewritten to use ModAPI, and everything should be merged to dev. I'm planning on merging to master fairly soon, if I get the time to finish the remaining required changes.

mkumpan commented 6 years ago

Kinda. I'm a corpmate of his acquaintance. :)

This particular PR is directed at issues we were having with corrupted userdata.json due to lack of disk space.

My next issue will be debugging of our failing killmail feed - the jsons appear in the logs just fine so far, but the bot is not reporting them, by the looks of it.

I will also want to add the ability (if not yet present) to accept messages for discord from a different tool, hosted on the same machine. (Going for a custom fleet calendar tool, but I don't want to do it in node, and Java integration with Discord API appears to be nonexistent)

Anyways, if you setup a list of core tasks in the github issues tool, I can help you get things moving.

Protected commented 6 years ago

AWRyder isn't currently interested in spending time on Eve. If you're willing to put in the effort, I can coordinate with you on this, but please confirm before I spend the time writing the issues!

Current, the eve branch is almost a fork. What's important for me, if you want to merge both, is for the module to start relying on other modules for common functionality, like ModAPI and ModLogger, instead of being monolithic and reinventing the wheel. This might in turn require rewriting or improving ModAPI (and other dependencies).

The differences in ModUsers and ModCommands between both branches are minimal, so you should be able to reimplement your changes on top of the current latest versions when merging. However, you will find JSON data file saving and loading on Module.js . It's reused by all modules.

Comments about the pull request itself:

Do not output anything to the console, use the logger provided by Module (this.log). Respect the "quiet" flag that can be passed to the save function in the latest commit in dev.

Please cap the amount of backups per datafile using a main config file parameter, 0 to disable. It may also be interesting to provide a separate path for backups (optional, keep current behavior if missing) in the paths map.

I don't disagree that it would be a good idea to use asynchronous saving for JSON data files (I've had that notion in the back of my mind for a while), but since we currently share the code for multiple modules, this may result in a large overhaul of half of the application. You may find this to be quite a lot of work. It's important to check on a module by module basis if the asynchronicity won't break anything (hopefully it won't).

Rejection must be handled for all promises, unhandled rejections are currently deprecated. I think it's more effective to handle the rejection in the save function in Module.js and log any errors.

I would prefer if you kept "private" module functions as class methods, just a matter of style consistency though. It allows you to call upstream methods for accessing dependencies, the logger and so on by using 'this' without having to pass a reference to the module class into your function.

Likewise, I would prefer to avoid "do a bit of everything" dependencies like lodash outside modules that are fully maintained by someone else (you can keep it in ModEveRoles).