Conversation
There was a problem hiding this comment.
The provenance of the branch looks good now, but it looks like your first commit (0793d5e) piggy-backed this file unintentionally. We can talk about how that might have happened this time and what can be done to avoid it, but in terms of finalizing this PR, can you redo the branch one more time and make sure that your committed changes have just your intended changes exclusively. The redo will also help you determine how/why this file found its way to your commit
| if (anyExpressionChecked) { | ||
| source = $("input[name=expressionSource]:checked")[0].value; | ||
| if (source === "none") { | ||
| source = null; | ||
| } else if (source === "userInput") { | ||
| // only demos will have an expression source | ||
| source = grnState.workbook.expression.source | ||
| ? grnState.workbook.expression.source | ||
| : "user-data"; | ||
| } | ||
| } else { |
There was a problem hiding this comment.
For now, we can keep this code as-is. But for future reference—and we’ll see if there’s time to discuss this at the meeting—what the code is doing here represents a higher-level programming pattern of imperative code whose sole result is the value of a single variable (source). When you have code that does this, there is an alternative approach that does not rely on side effects (and again if you’re unfamiliar with the term, we can discuss that)
In broader practice, this latter alternative (which will also be good for Alex to know) is generally considered safer and more robust, so even though what you have here is functionally equivalent, in terms of programming craft, the alternative is recommended. We can discuss this at some future juncture; it doesn’t need to block this PR
|
OK, so I see now what happened, which is that you did a But also stepping back, note the context of the instruction: the purpose was to produce a clean On the other hand, you also have agency here and can continue to hold your own counsel as you follow steps. This summer work is a great opportunity for you to advance in that regard |
|
This has been superseded by #1609, so closing this one directly |
Pull Request Checklist