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

Simplify and clarify config #40

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

abigailwillow
Copy link
Contributor

@abigailwillow abigailwillow commented Jul 20, 2024

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 torikushiii marked this pull request as ready for review July 22, 2024 11:44
@torikushiii
Copy link
Owner

whoops, i clicked the wrong button 😓

@torikushiii torikushiii marked this pull request as draft July 22, 2024 11:45
@torikushiii
Copy link
Owner

torikushiii commented Jul 22, 2024

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
Copy link
Contributor Author

abigailwillow commented Jul 22, 2024

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.

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

Successfully merging this pull request may close these issues.

2 participants