Skip to content

Conversation

@rajun274
Copy link

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
Copy link
Owner

tomayac commented Aug 30, 2021

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

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 WIKIDATAQID_NOTARRAY_, then I think this idea is the right approach.

Let me know your thoughts. Thanks!

@tomayac
Copy link
Owner

tomayac commented Aug 31, 2021

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

@rajun274
Copy link
Author

rajun274 commented Aug 31, 2021

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
Copy link
Owner

tomayac commented Aug 31, 2021

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

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