tomayac / wikipedia-tools-for-google-spreadsheets

Wikipedia Tools for Google Spreadsheets — Install:
https://gsuite.google.com/marketplace/app/wikipedia_and_wikidata_tools/595109124715?pann=cwsdp&hl=en
Apache License 2.0
141 stars 32 forks source link

Support 2D array inputs for WIKIDATAQID. #43

Open rajun274 opened 3 years ago

rajun274 commented 3 years ago

This is related to #41

This is the first PR for broad support for your Wikipedia functions to support array inputs.

The goal of this PR is to establish a baseline for how we'd like each function to be updated, and then update many more.

FYI, this is my first ever pull request :)

tomayac commented 3 years ago

This looks like it works already. Maybe an alternative to having a no-array variant for each function could be the following pattern:

function(input) {
  if (!Array.isArray(input) {
    input = [input];
  }
  /*…*/
}

So the function would assume an array by default, and if it's passed a non-array would convert it on the fly. I'm not 100% sure if this would be feasible if there are other arguments.

rajun274 commented 3 years ago

Thanks for the review Thomas!

From your recommendation snippet, I'm assuming the /*..*/ would be your existing code in WIKIDATAQID? I think the full code would be this:

function WIKIDATAQID(article) {
  if (!Array.isArray(article)) {
    article = [[article]];
  }
  'use strict';
  if (!article) {
    return '';
  }
  var results = '';
  try {
    var language;
    var title;
    if (article.indexOf(':') !== -1) { <-- PROBLEM
  ...

The problem is all the existing code assumes non-array, and so all existing code would need to be wrapped in a for loop in a for loop, with an outer 2D array to capture results. It's a bit clunky?

Another alternative is to wrap your existing code with a function that's defined inside the outer function. Example:

function WIKIDATAQID(input) {
  function process(article) {
    // All the existing code you've written.
  }
  return Array.isArray(input) ?
      input.map(row => row.map(cell => process(cell))) :
      process(input);
}

This saves the definition of an awkwardly-named function. If your concern is the awkwardly-named function WIKIDATAQIDNOTARRAY, then I think this idea is the right approach.

Let me know your thoughts. Thanks!

tomayac commented 3 years ago

Then I think going with the nested process function is the way to go. Thanks for the thoughts you are putting into this!

rajun274 commented 3 years ago

Updates to my PR:

  1. I've created the inner function process inside WIKIDATAQID.
  2. For completely unknown reasons, my initial changes to Tests.gs.js were bad :( My initial code didn't work b/c some of your functions already return 1D arrays. I've updated the code so that tests pass.
tomayac commented 3 years ago

It looks like this is good to go. Do you prefer to open PRs for each function, or have one large PR (my preference).