Add new tests to check for if a file is blank issue #1602

[JustAlex3125]

This file will break, and the tests will fail, but I am currently working on the fix to make sure that these tests pass.

[coveralls]

coverage: 82.188% (+0.4%) from 81.764% — alex/property_undefined_error_#1602 into beta

[dondi]

Looks like the overall functionality is where we want it, but the under-the-hood implementation is a learning opportunity—see the notes for additional details

Ultimately, we want to keep the intended behavior, but how we achieve it is best done in a manner that this particular language (JavaScript) mostly does it (again see the details)

[dondi]

This works in principle, but we’ll want to revise the approach due to something we can talk about more at the next meeting, which is the “culture” of a certain language. “Culture” here means the typical idioms and approaches that gravitate more to certain languages vs. others. It’s a somewhat heady concept but it becomes clearer as one learns more and more programming languages

Here, the idiom at play is when to use a try clause to handle an error after the fact vs. using if to check for a potential error before it happens. Although both approaches can function correctly in multiple languages, some languages are inclined more toward one approach vs. the other. In this case, the former is something that Python gravitates more toward, but JavaScript leans toward the latter

So here, let’s explore what the equivalent of this statement would be via an if statement. How would you “rephrase” this so that it looks like this instead:

Suggested change

	try {
	idLabel = sheet.data[0][0];
	} catch (err) {
	addExpError(expressionData, constants.errors.emptyWorkbookError());
	return expressionData;
	}
	if (/* what condition is equivalent to sheet.data[0][0] throwing an error? */) {
	addExpError(expressionData, constants.errors.emptyWorkbookError());
	return expressionData;
	}

This approach is more idiomatic to JavaScript that a try clause (for this situation)

[dondi]

This is similar—how would you approach this as an if instead of a try?

[dondi]

I’m unclear on the assignment here; what role does the workbookType variable play in the overall code? An assignment to something that is being returned anyway is generally looked at as an anti-pattern. It feels like this can just be a return?

Suggested change

	return (workbookType = undefined);
	return;

[dondi]

Great, we’re very close! Just one last loose end coming out as a consequence of the new if check—and interestingly, this one reduces the number of changes needed in the PR 😁

[dondi]

Thanks for this! Yes, this is more “JavaScript-like” than “Pythonic.” In JavaScript, there are multiple variants for this condition also, like with using the optional operator (?), and we can explore those as we proceed. This approach works perfectly well, particularly because JavaScript does Boolean short-circuiting (and if that is unfamiliar to you, you’ll get to it eventually or I can also talk you through it)

With all that said, there is one last adjustment: notice that, with this change, there is no longer a need to declare idLabel at the point where it’s being done here. The declaration can be done after the if statement, when we are sure that sheet.data[0][0] is legit and so… (see comment below)

[dondi]

Oops missed one more thing—the first clause is best written as !sheet.data

[dondi]

…you can delete lines 60, 61, and 68, and revert back to this line, which will now be guaranteed to not throw thanks to the if check

[dondi]

Same here—you can restore this line; just make sure it happens after the if guard

[JustAlex3125]

After my discussion with @kdahlquist I realized that this fix while does fix the issue does not display the proper warnings and errors for expression sheets and network sheets respectively. This issue is not ready to be pushed to beta yet. Once I correct the warnings and errors than it should be good and ready for review again.

[dondi]

Thanks for the updates! I saw your most recent comment about how this PR needs more work. The previously-requested changes have indeed been applied correctly, so that’s good. lmk when this is ready for beta. Thanks!