Skip to content

Conversation

Fryguy
Copy link
Member

@Fryguy Fryguy commented Aug 19, 2025

This PR removes setting a giant map of constants, and then destructuring them into individual constants in favor of just setting the individual consts directly.

@asirvadAbrahamVarghese Please review. (I wasn't sure if there was a reason it was doing this double settings, but for me this is half as many lines for the same result.

@asirvadAbrahamVarghese
Copy link
Contributor

Honestly, I'm still not sure which approach is better. I just compared the pros and cons from WCA and ended up choosing the object structure. My thinking was - if we later move these to a separate constants file, it’s easier to import a single textConstants object. That said, when using an object, destructuring takes up more lines, but it helps keep things grouped. On the other side, exporting individual variables can may be lead to namespace clutter.
But now that I think about it, we’re probably not moving these to another file anyway, so let’s stick with the approach you’ve used here. I might just rename them using ALL_CAPS with underscores to make them stand out more, like @jrafanie suggested here

No problem, I’ve already got a refactor PR open for the schedule form (#9549), so I’ll include it there. Will open another for the collect-logs form.

If you're good to merge this, I’ll rebase and do the ALL_CAPS renaming.

@asirvadAbrahamVarghese
Copy link
Contributor

This change will be covered from:
Collect-logs: #9575
Schedule: #9549

@Fryguy
Copy link
Member Author

Fryguy commented Aug 22, 2025

Closing in favor of #9575 and #9549

@Fryguy Fryguy closed this Aug 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants