Skip to content

refactor dynamic_forms.js to be more intuitive (no token)#5141

Closed
simonLeary42 wants to merge 1 commit into
OSC:masterfrom
simonLeary42:simplify-id-from-token
Closed

refactor dynamic_forms.js to be more intuitive (no token)#5141
simonLeary42 wants to merge 1 commit into
OSC:masterfrom
simonLeary42:simplify-id-from-token

Conversation

@simonLeary42
Copy link
Copy Markdown
Contributor

@simonLeary42 simonLeary42 commented Mar 10, 2026

parseMinMaxFor uses the variable name tokens to describe regex match groups, but I think that's confusing since the word "token" already has a special meaning in this file. So I changed it to groups.

makeChangeHandlers uses the variable name token to describe a substring of an option-for directive, but I think that's confusing because it contains other characters as well as the token.

@github-project-automation github-project-automation Bot moved this to Awaiting Review in PR Review Pipeline Mar 10, 2026
@simonLeary42 simonLeary42 changed the title Simplify id from token refactor dynamic_forms.js to be more intuitive (idFromToken) Mar 10, 2026
@simonLeary42 simonLeary42 force-pushed the simplify-id-from-token branch from 722caf1 to ea2cd27 Compare March 10, 2026 17:34
@simonLeary42 simonLeary42 force-pushed the simplify-id-from-token branch from ea2cd27 to 2565fc2 Compare March 10, 2026 17:43
@simonLeary42 simonLeary42 changed the title refactor dynamic_forms.js to be more intuitive (idFromToken) refactor dynamic_forms.js to be more intuitive (no token) Mar 10, 2026
@simonLeary42
Copy link
Copy Markdown
Contributor Author

I reverted the function name changes, and I broke off #5146 and #5145 into separate PRs.

@johrstrom
Copy link
Copy Markdown
Contributor

Yea I'm not so sure on this one, "token(s)" generally mean "thing(s) you've parsed" which I think fits here as we're parsing these values from javascript data fields.

Indeed you're passing the variable to idFromToken suggesting that the thing you're passing is a 'token' (something you've parsed from another string).

@simonLeary42
Copy link
Copy Markdown
Contributor Author

👍

@github-project-automation github-project-automation Bot moved this from Awaiting Review to Merged/Closed in PR Review Pipeline Mar 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants