Various fixes and cleanups

[rowanthorpe]

I have been working on-and-off on implementing 'blacklisting' as described here, and in anticipation of that, but also just from having to re-familiarize after a while away from the code, I found various improvements/fixes/clarifications which I am bundling in a PR rather than driving you nuts with a string of PRs. If any commit-messages are not self-explanatory enough let me know.

[GioBonvi]

I fuond two semistandard errors. Everything else looks OK.

I'll run some tests later on.

[GioBonvi]

My semistandard test throws an error here:

file: 'code.gs'
severity: 'Error'
message: 'Unnecessary use of conditional expression for default assignment. (no-unneeded-ternary)'
at: '769,29'
source: 'semistandard'
code: 'no-unneeded-ternary'

It looks like in the semistandard spec the no-unneeded-ternary rule is configured with "defaultAssignment": false (see ESLint documentation).
Adding /* eslint no-unneeded-ternary: ["error", { "defaultAssignment": false }] */ at the top of the file fixes this problem

[GioBonvi]

Another simpler problem with semistandard here: a space is required between "isIn" and the parethesis.

[rowanthorpe]

Oops, sorry about that. I had done semistandard testing, but not at the end after the last changes. Serves me right for opening a PR late at night while half-asleep. I fixed the two changes you pointed out (thanks) and also noticed that part of the jsdoc-comment I'd added for the new function should have just been an internal devel-comment, so changed that too.

[rowanthorpe]

I just sneakily added 4 more small "general cleanup/readability" commits to this PR. These are the last pre-blacklisting-functionality commits. In a moment I will open the blacklisting PR separately, but based against the HEAD of this branch rather than present development-HEAD, so there is no unnecessary rebasing back-and-forth.

[GioBonvi]

This will not work since Plus.People.get(gPlusProfileId) throws a 404 error when gPlusProfileId is not valid, causing the whole script to crash instead of simply aborting the execution of MergedContact.prototype.getInfoFromGPlus().

I guess you saw the if (gPlusProfile === null) and thought that was what the function returned in case of an error; at least this is what I immediately thought reading this code right now.
I say right now because I was the one who wrote those exact lines 7 months ago (ff8d890), but, for the life of me, I cannot remember why would I've had to put that silly if condition that will never be met.

Looking at the documentation I see no way for that API call to return null, so that if clause seems to be overkill and misleading at the same time.
I am running some tests to verify this: will report back as soon as they are done.

[rowanthorpe]

Ah, OK (especially because it does seem logical for a web-request to return 404 to signify not found). In that case, for the sake of consistency maybe it is better to make the "get" inside getInfoFromContact() use the same throw/catch pattern (also in case they start throwing 404 errors for edge-cases there too...). Either way I think it is worth prefixing the throw-catch with a comment explaining why (about the 404, with the wording you just used in your comment) so we don't have the same dialogue every 4-5 months, or however long it takes one of us to forget the rationale again (or a new developer to not know about it). While you look into whether the "if ... null" bit is actually needed inside the throw-catch, I will revert those lines (and the getInfoFromContact() one) to throw-catch with explanatory comments, and rebase-and-push.

[GioBonvi]

Whoops, run my tests but forgot to report back.

I run the script on all my contacts, but while many of them caused a 404 error due to invalid IDs (I think they are old accounts now deleted or something similar) none of them caused the API to return null.

Having fixed this problem I think we can merge this, right?

[rowanthorpe]

At present in 0f74a9a, I've just changed the other mentioned fetch/exception/null case to be the same (i.e. wrapped in try-catch, with the error-throw if null). I suspect the check for null might be a leftover of copy-pasta from invocations of LocalCache.retrieve(url, retry) because that returns null after there have been retry number of failed retries. It seems for the sake of this PR it would be safer for now to just leave the null-checks in place because at worst they will be redundant.

Looking ahead however, I noticed while checking on this that there are various "fetch from API" and "parse from raw input" points in the code with similar-but-different variations on the exception/error/null-handling (see snippets at the bottom of this comment). I suggest it would be good to standardize use of LocalCache.retrieve() for some of these, and standardize the error/exception-handling within that, to be more DRY, to make the code more readable/intuitive (e.g. reasoning behind the null-check), and of course to speed up any repeat-retrievals due to caching. For example the function-signatures of retrieve() and fetch() could be changed as follows (you can use the "default arg" syntax in ES6 so I guess that is OK in Google Apps Script, otherwise we use one of the slightly less concise solutions for it shown at that stackoverflow question).

Change from:

//// definition
LocalCache.prototype.fetch = function (url, retry) {...}
LocalCache.prototype.retrieve = function (url, retry) {...}

//// usage
// [1] (using cache)
inlineImages['contact-img-' + imgCount] = cache.retrieve(self.data.getProp('photoURL')).getBlob().setName('contact-img-' + imgCount);
// [2] (not yet using cache)
googleContact = ContactsApp.getContactById('http://www.google.com/m8/feeds/contacts/' + encodeURIComponent(settings.user.googleEmail) + '/base/' + encodeURIComponent(contactId));
// [3] (not yet using cache)
response = JSON.parse(response);

Change to:

//// definition
LocalCache.prototype.fetch = function (url, retry, getFunc=UrlFetchApp.fetch) {...}
LocalCache.prototype.retrieve = function (url, retry, getFunc=UrlFetchApp.fetch) {...}

//// usage
// [1] (using cache, same as before)
inlineImages['contact-img-' + imgCount] = cache.retrieve(self.data.getProp('photoURL')).getBlob().setName('contact-img-' + imgCount);
// [2] (using cache, specifying custom function & 2 "tries")
googleContact = cache.retrieve('http://www.google.com/m8/feeds/contacts/' + encodeURIComponent(settings.user.googleEmail) + '/base/' + encodeURIComponent(contactId), 2, ContactsApp.getContactById);
// [3] (using cache, specifying custom function & 1 "try")
response = cache.retrieve(response, 1, JSON.parse);

Please let me know if you agree. If so, I will open a separate PR to do that. Of course some points in the code are probably better off not being cached if there will be so many different requests that the cache-object would fill up the process memory.

---

Here are the "various snippets" mentioned above:

//////// at line 241 - in LocalCache.fetch() which is used internally by LocalCache.retrieve()
    try {
      response = UrlFetchApp.fetch(url);
      if (response.getResponseCode() !== 200) {
        throw new Error('');
      }
      // Break the loop if the fetch was successful.
      break;
    } catch (error) {
      errors.push(error);
      response = null;
      Utilities.sleep(1000);
    }

//////// at line 400
  try {
    googleContact = ContactsApp.getContactById('http://www.google.com/m8/feeds/contacts/' + encodeURIComponent(settings.user.googleEmail) + '/base/' + encodeURIComponent(contactId));
    if (googleContact === null) {
      throw new Error('');
    }
  } catch (err) {
    log.add('Invalid Google Contact ID or error retrieving data for ID: ' + contactId, Priority.INFO);
    return;
  }

//////// at line 489
  try {
    gPlusProfile = Plus.People.get(gPlusProfileId);
    if (gPlusProfile === null) {
      throw new Error('');
    }
  } catch (err) {
    log.add('Invalid GPlus Profile ID or error retrieving data for ID: ' + gPlusProfileId, Priority.INFO);
    return;
  }

//////// at line 631
        try {
          // Get the default profile image from the cache.
          inlineImages['contact-img-' + imgCount] = cache.retrieve(self.data.getProp('photoURL')).getBlob().setName('contact-img-' + imgCount);
          line.push('<img src="cid:contact-img-' + imgCount + '" style="height:1.4em;margin-right:0.4em" />');
        } catch (err) {
          log.add('Unable to get the profile picture with URL ' + self.data.getProp('photoURL'), Priority.WARNING);
        }

//////// at line 1744
  try {
    response = cache.retrieve(baseGitHubApiURL + 'releases/latest');
    if (response === null) {
      throw new Error('');
    }
  } catch (err) {
    log.add('Unable to get the latest version number', Priority.WARNING);
    return false;
  }

//////// at line 1754
  try {
    response = JSON.parse(response);
    if (typeof response !== 'object') {
      throw new Error('');
    }
  } catch (err) {
    log.add('Unable to get the latest version number: failed to parse the API response as JSON object', Priority.WARNING);
    return false;
  }

//////// at line 1773
  try {
    return (version).compare(new SimplifiedSemanticVersion(latestVersion)) === -1;
  } catch (err) {
    log.add(err.message, Priority.WARNING);
    return false;
  }

//////// at line 1869
  try {
    if (Calendar.Calendars.get(settings.user.calendarId) === null) {
      throw new Error('');
    }
  } catch (err) {
    log.add('Your user.eventSource setting is invalid!', Priority.FATAL_ERROR);
  }

//////// at line 1984
  try {
    eventCalendar = Calendar.Calendars.get(calendarId);
    if (eventCalendar === null) {
      throw new Error('');
    }
  } catch (err) {
    log.add('The calendar with ID "' + calendarId + '" is not accessible: check your calendarId value!', Priority.FATAL_ERROR);
  }

//////// at line 1994
  try {
    startDate = Utilities.formatDate(eventDate, eventCalendar.timeZone, 'yyyy-MM-dd\'T\'HH:mm:ss\'Z\'');
    endDate = Utilities.formatDate(new Date(eventDate.getTime() + 1 * 60 * 60 * 1000), eventCalendar.timeZone, 'yyyy-MM-dd\'T\'HH:mm:ss\'Z\'');
    log.add('Looking for contacts events on ' + eventDate + ' (' + startDate + ' / ' + endDate + ')', Priority.INFO);
  } catch (err) {
    log.add(err.message, Priority.FATAL_ERROR);
  }

[GioBonvi]

This opens a whole new can of worms

• I agree that fetch/exception/null handling is quite messy right now and we should adopt a standard to deal with it.
• As far as I know Google Scripts runs on an ES5 Javascript engine, so we are not allowed to use ES6 syntax. Copying that example function from Stack Overflow and placing it in the code will trigger an error when saving since it will not be recognized as valid code. Like you said, however, that's not a problem since we can use the ES5 syntax for default parameters (we actually already use it in various places such as LocalCache.prototype.fetch definition).
• I am not too sure that adapting the LocalCache.retrieve() for all those cases you listed would be the best idea; I would keep the LocalCache strictly as a (string ResourceURL) -> (Object data) (or at least (string ResourceID) -> (Object data), I'll explain more about this later). I think using it in the context of JSON.parse() is a bit of a strech, but I'd like to hear a more detailed explaination from you since I have the feeling I am probably missing some crucial part of your reasoning.
  I have an alternative proposal regarding this whole LocalCache/fetch/error/null management which would probably better discussed in its own issue since this PR is becoming a little too broad in scope and this change would be quite important both in terms of size and functionality. If you agree I would merge this PR and open a new issue to discuss this new topic more thoroughly (or if you feel like this PR should wait until we have sorted this new problem out we can simply leave it open until then).

[rowanthorpe]

I agree it is better to merge this PR and resolve the remaining issue separately, also because merging this PR as-is allows merging the "event blacklisting" without rebasing.

Just to clarify what I was suggesting on this thread anyway though, I suggest just adding an optional third argument to LocalCache.prototype.fetch() (and identically to LocalCache.prototype.retrieve()) so that with 2 arguments both methods perform exactly the same as before, but when there is a 3rd argument it overrides UrlFetchApp.fetch as the "fetch function". The nice thing about the "fetch calls" in those snippets is that they all take one argument (see below), so whenever we want to cache one of them (not necessarily always, as you pointed out) we could just inject the LocalCache.prototype.retrieve() method in its place, passing the existing function as third argument (and a second argument - retries - of 1 if just a one-shot, perhaps 2 or 3 for API calls). For the places where it is silly or counterproductive to do actual caching, it just means we have to clean up the exception/error-handling similarly in each place (at the cost of a little repetition), and for the ones worth caching we only need to cleanup the code in one place in those LocalCache methods.

Another approach could be to add a 4th argument, which is a boolean to disable actually storing the result in cache. This would allow making all get/fetch/parse requests go through the same method, optionally leveraging the same retries functionality and the write-once-use-always error/exception/null-handling. Of course, then it starts to have some semantic drift when you use a cache object to do a "no cache" fetch...

The following all take one arg and return one var so could have the caching injected where desired for those. Of course it is probably not worth caching some of them, but we can decide that as we go:

response = UrlFetchApp.fetch(url)
// ----v
response = cache.retrieve(url) // default function, no need for additional args

googleContact = ContactsApp.getContactById(
  'http://www.google.com/m8/feeds/contacts/' + encodeURIComponent(settings.user.googleEmail) + '/base/' + encodeURIComponent(contactId)
)
// ----v
googleContact = cache.retrieve(
  'http://www.google.com/m8/feeds/contacts/' + encodeURIComponent(settings.user.googleEmail) + '/base/' + encodeURIComponent(contactId),
  2,
  ContactsApp.getContactById
)

gPlusProfile = Plus.People.get(
  gPlusProfileId
)
// ----v
gPlusProfile = cache.retrieve(
  gPlusProfileId,
  2,
  Plus.People.get
)

response = JSON.parse(
  response
)
// ----v
response = cache.retrieve(
  response,
  1,
  JSON.parse
)

eventCalendar = Calendar.Calendars.get(
  calendarId
)
// ----v
eventCalendar = cache.retrieve(
  calendarId,
  2,
  Calendar.Calendars.get
)