ref: Check config at trial level
#357
Conversation
This was the original name of function. Returning to that name as it's more prescriptive, will wait to change until the bigger change in v4
RobertGemmaJr
requested review from
hollandjg,
digicosmos86,
broarr and
galenwinsor
| function buildHoneycombBlock(jsPsych) { | ||
| const { honeycomb: honeycombSettings } = taskSettings; | ||
| const honeycombSettings = taskSettings.honeycomb; |
There was a problem hiding this comment.
Nitpicky but it might be easier to read if config variables are in caps?
There was a problem hiding this comment.
Definitely on my nice to haves but I'm not sure where to draw the line. Maybe just SETTINGS, LANGUAGE, and ENV? At the very least I think env isa much better name than config
| @@ -35,7 +43,7 @@ function buildHoneycombBlock(jsPsych) { | |||
| }, | |||
| // Conditionally flashes the photodiode when the trial first loads | |||
| on_load: () => { | |||
| if (config.USE_PHOTODIODE) pdSpotEncode(eventCodes.honeycomb, 1, config); | |||
| if (config.USE_PHOTODIODE) pdSpotEncode(eventCodes.honeycomb); | |||
There was a problem hiding this comment.
Hmm I guess that would make this look weird, so maybe never mind.
| /** Builds the blocks of trials needed to start and setup the experiment */ | ||
| function buildPreambleBlock() { | ||
| const timeline = [nameTrial, enterFullscreenTrial, welcomeTrial]; |
There was a problem hiding this comment.
I like this pattern a lot! You're basically moving things from the top level into functions, right?
There was a problem hiding this comment.
Right. And there's a HUGE difference between code that's inside a function and code that's inside a trial's callback function. Hoping to be a little more clear about that.
What's Changed
configchecks are done at the trial level, not insidephotodiodeGhostBoxandpdSpotEncodebuild___Trial___TrialpdSpotZEncodewhen it's being calledexport defaultfrom trial filesReasoning
configResponsibilityThis is one that I'm not entire sure which is better. I've chosen to have the trials check the
configobject rather than internal lib functions -photodiodeGhostBoxandpdSpotEncode.Part of the purpose/magic of HC is that it takes away from the developers need to worry about which environment the code is running in, which is why I do think having the config checks hidden inside the functions may be better. But ultimately a lot of this code is changing soon (because the event markers and photodiode will be available in both the browser AND native app) and I think expecting the caller to chose what they do/don't want to execute is best. This code is moving in that direction.
Variables vs Functions
JsPsych expects all trials as an object. There are certain instances where we need to build a trial and must use a function (e.g. when we pass
jsPsycharound. However, in most cases, it's simpler to leave as a const variable.Note that some keys to the trial (e.g.
stimulus,prompt, etc.) can be written as callback functions that return the required type! This closer matches the jsPsych documentation and generally seems to be best practice, rather than functions that create the entire trial object. This PR covers a lot of those cases but doesn't cover 100%. I'll keep making changes as I progress.Naming
build___Trialis a bit more idiomatic of the difference between the variables and functions. Code that is a function is NOT a trial, it's building the trial object___Trialvariable naming is a bit pedantic but I think it better communicates exactly what the objects are in a way that pure JS is not the greatest at. I think it's a useful naming scheme, especially considering HC will be most lab members first time writing codeRemoval of
export defaultThis is just to keep consistency between files. I'm not 100% sure if I want one trial per file but I'll think about that for another PR. For now it makes it so the trials are imported in the same way across all trials