Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref: Refactor task settings #375

Merged
merged 4 commits into from
Jan 9, 2024
Merged

Conversation

RobertGemmaJr
Copy link
Member

@RobertGemmaJr RobertGemmaJr commented Dec 13, 2023

What's Changed

  • config.json is renamed settings.json
  • taskSettings export is named SETTINGS

Reasoning

Name Change

The config.json file holds configuration settings specific to the task. I think it's better to name as settings since we have multiple configuration settings (language, environment variables, and this) - it's a bit more specific.

This is one move towards that name change, and since they're just re-exports of the json I thought all caps was in order. Hence LANGUAGE and SETTINGS.

Removing Settings in JS

I decided to remove the cascading of trial settings from main.js. I do still want this cascading effect to take place but that situation is specific to task settings that are stored in Firebase. I think only one base/fallback settings in the main repo is okay and, if so, the separate JSON files is the better place for them.

Reviewer Qs

  • Do you think having the single point of truth for the trial settings as a json file is a good idea?

@RobertGemmaJr RobertGemmaJr added the 3.3 Versioning: Issue in regards to version 3.3 release label Dec 13, 2023
@RobertGemmaJr RobertGemmaJr self-assigned this Dec 13, 2023
Copy link

github-actions bot commented Dec 13, 2023

Visit the preview URL for this PR (updated for commit 32bcffb):

https://ccv-honeycomb--pr375-ref-tasksettings-m7b73fyi.web.app

(expires Wed, 20 Dec 2023 21:08:34 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 4ace1dcea913a952d2a1af84b94a4421bf36e610

Copy link
Contributor

@galenwinsor galenwinsor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good! I'm not entirely sure what you mean by the "cascade" of settings. But I think having a single source of truth in a JSON is great, if you don't need to change the settings frequently.

Copy link

@hetd54 hetd54 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also a fan of your description above! Agree with Galen 👍

Copy link

@broarr broarr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, though I'm not sure I have all the context required. Why are SETTINGS and LANGUAGE the caps and the rest of the imports from config camelCase?

@RobertGemmaJr
Copy link
Member Author

Why are SETTINGS and LANGUAGE the caps and the rest of the imports from config camelCase?

@broarr As of right now SETTINGS and LANGUAGE are the only ones from JSON files which is why I made them caps. In the end, yes, I think everything from this file should be exported as call caps.

Some of the exports will be removed in v3.3 and I'd like to actually remove the config option (and export things like USE_ELECTRON individually). Definitely a work in progress!

@RobertGemmaJr
Copy link
Member Author

I'm not entirely sure what you mean by the "cascade" of settings.

The JSON will be the starting source of truth but the settings will be able to be configured on a study/participant level in Firebase. It's not implemented yet though.

@RobertGemmaJr RobertGemmaJr merged commit 21a3a98 into hold-cleaning Jan 9, 2024
8 checks passed
@RobertGemmaJr RobertGemmaJr deleted the ref-taskSettings branch January 9, 2024 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.3 Versioning: Issue in regards to version 3.3 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants