search

torikushiii / hoyolab-auto

Auto check-in and others for any Hoyoverse games
https://ko-fi.com/torikushiii
GNU Affero General Public License v3.0
107 stars 24 forks source link

Simplify and clarify config #40

Open abigailwillow opened 4 months ago

abigailwillow commented 4 months ago

This is a draft PR that I've been working on to suggest a simplified and clarified configuration structure. I'd love to discuss these options and see if we can move ahead with any or even all of these changes. This PR is not meant to be directly implemented as is and needs lots of work to be fully functional.

Overview

  1. All casing was changed from camelCase to snake_case as I believe this to be easier to read.
  2. The platforms section was renamed to integrations as I believe this is a clearer description of the contained data.
  3. The integrations section can now contain a multitude of different types of integrations to facilitate splitting notifications and commands.
  4. The crons section was moved down below the accounts section, as users are less likely to want to change crons, whereas the accounts section I believe to be a higher priority.
  5. The accounts section was completely revamped. Each account contains a single cookie and can be configured per game, instead of having a separate cookie for each game for easier maintainability.
  6. Entries in the accounts section now contain an integrations section which contains an array of integrations to use in accordance with the previously specified integrations entries. For example, this would allow users to split Discord Webhook notifications for two separate accounts between two separate channels.
  7. The crons section no longer contains a whitelist and blacklist entry.
  8. The crons entries are now objects with an explicit enabled flag, which I believe to be clearer than a blacklist and whitelist system.
torikushiii commented 4 months ago

whoops, i clicked the wrong button 😓

torikushiii commented 4 months ago

1. All casing was changed from camelCase to snake_case as I believe this to be easier to read.

I think I'm gonna stick with camelCase with this one to minimize disruption for current users.

2. The platforms section was renamed to integrations as I believe this is a clearer description of the contained data. 3. The integrations section can now contain a multitude of different types of integrations to facilitate splitting notifications and commands. 6. Entries in the accounts section now contain an integrations section which contains an array of integrations to use in accordance with the previously specified integrations entries. For example, this would allow users to split Discord Webhook notifications for two separate accounts between two separate channels.

I'm not sure renaming platforms to integrations is worth the trouble just yet since platform is good enough, but sounds kinda nice to let users pick accounts for notifications. Maybe we can revisit this whole integrations thing.

4. The crons section was moved down below the accounts section, as users are less likely to want to change crons, whereas the accounts section I believe to be a higher priority. 7. The crons section no longer contains a whitelist and blacklist entry. 8. The crons entries are now objects with an explicit enabled flag, which I believe to be clearer than a blacklist and whitelist system.

Moving accounts above crons makes sense, and I agree. However, I'm not sold on ditching the whitelist/blacklist for crons just yet. It's super easy for users to quickly enable or disable crons that way. Your "enabled" flag idea works too, and I think it would be nice, but it might be more complicated for new or current users. But I'm all ears for suggestions.

6. Entries in the accounts section now contain an integrations section which contains an array of integrations to use in accordance with the previously specified integrations entries. For example, this would allow users to split Discord Webhook notifications for two separate accounts between two separate channels.

Letting users split notifications per account using different webhooks, like I said above, is a really neat idea! I'm just not sure about the best way to set that up in the config yet. Maybe we could add an array to each webhook token? That would mean reworking how the platform methods handle things though

abigailwillow commented 4 months ago

I think I'm gonna stick with camelCase with this one to minimize disruption for current users.

Mostly personal preference, camelCase is the convention as I've noted before. Again, wouldn't mind including this in a conversion script. Since I was already reworking the entire file I figured I'd at least suggest it.

I'm not sure renaming platforms to integrations is worth the trouble just yet since platform is good enough, but sounds kinda nice to let users pick accounts for notifications. Maybe we can revisit this whole integrations thing.

Also a matter of personal preference, I think integrations is more descriptive than platforms, but it's not really a huge deal. The main change here is to allow any amount of integrations with any account.

Moving accounts above crons makes sense, and I agree. However, I'm not sold on ditching the whitelist/blacklist for crons just yet. It's super easy for users to quickly enable or disable crons that way. Your "enabled" flag idea works too, and I think it would be nice, but it might be more complicated for new or current users. But I'm all ears for suggestions.

I strongly believe having an enabled flag that you set either to true or false is clearer, as users directly see which cron they're editing. Whereas with a blacklist and whitelist, I could think of a few potential user error-related issues, such as if a user would make a small typo and not notice. Whereas most editors will tell you if you misspelled true or false. Having the crons as a distinct object could also have benefits in the future, for example adding an array of which accounts to run the crons for.

Letting users split notifications per account using different webhooks, like I said above, is a really neat idea! I'm just not sure about the best way to set that up in the config yet. Maybe we could add an array to each webhook token? That would mean reworking how the platform methods handle things though

I hadn't yet looked at the implementation, and have been focusing on optimizing the config file as much as I could. I figured looping over the integrations on start-up and spinning up a module that runs an instance of the required integration would be feasible. I have not yet gone through the effort of implementing any of these changes, as I wanted to discuss the draft first.