GAS-ICS-Sync
GAS-ICS-Sync copied to clipboard
Beginning to refactor globals into locals
In response to #196, I started refactoring the code to remove several globals. I also removed some unnecessary conditional logic and optimized a handful of temporary variables. Gating logic has been moved into the email summary function, which also removed the need for the boolean. Lastly, I tweaked a few comments to be more consistent with the rest of the comments.
I tested the changes using 12 calendars in a variety of configurations along with added, modified, and removed events.
I added a couple comments, but otherwise I think it looks pretty good. @jonas0b1011001 to double-check functionality and see if he wants to add any comments as well
Looks fine to me. The only thing we are losing is the ability to enable "new version available" mails without receiving summary mails aswell. Not sure if we should keep the extra switch.
Looks fine to me. The only thing we are losing is the ability to enable "new version available" mails without receiving summary mails aswell. Not sure if we should keep the extra switch.
Maybe a configuration for each type of email is warranted. Something like enableUpdateEmail and enableSummaryEmail. For example:
var enableUpdateEmail = true; // Send an email if a new version of the script is available
var enableSummaryEmail = false; // Send a summary email after each run if anything changes
var email = ""; // To receive emails, an address must be provided
...
if(enableUpdateEmail) checkForUpdate();
...
if(enableSummaryEmail) sendSummary(email, addedEvents, modifiedEvents, removedEvents);
...
That's what i was thinking. I'm happy with your example.
I think one thing helpful to note here is that you could just use the bool and use Session.getActiveUser().getEmail() to get the email address. Though I don't know if that would cause more perm requests (related to #123)
I think it's better to make more of it as automated as possible, which would include things like Session.getActiveUser().getEmail(). However, users may want to send the notifications to another email address, so a way to choose would be good. I'm going to submit a different PR with a few tweaks to the notification flow, which I didn't think were a good fit for this PR due to the more limited scope.