-
Notifications
You must be signed in to change notification settings - Fork 59
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
feat(twilio-run): restructure configuration #198
Conversation
aa8884a
to
e44cbad
Compare
A lot of this seems to be the consolidation of flags, which is a nice refactor. Can you point out the parts where the |
The big changes are:
And yes I consolidated all flags. That should hopefully help us also use flags in a more consistent matter. |
return [ | ||
'{', | ||
`\t"commands": {},`, | ||
`\t"environments": {},`, | ||
`\t"projects": {},`, | ||
lines, | ||
'}', | ||
].join('\n'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to not use JSON.stringify
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I'm adding comments since it's JSON5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, in templateFlagAsConfig
, got you. Shame JSON5 doesn't make it easier to actually work with the comments it permits.
Splitting this to the two files, publicly shareable and local data, is definitely going to be a good thing. You were right in your comment about this being a difficult part of the code base! I can't see anything wrong, but you know it better than me. You have this as draft. Was there more work to be done? |
e44cbad
to
2298573
Compare
I'm still adding tests and I want to have a graceful migration option as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a couple of questions. I'm sure nothing is a blocker.
packages/twilio-run/__tests__/config/utils/mergeFlagsAndConfig.test.ts
Outdated
Show resolved
Hide resolved
This PR aims to restructure the way configuration is handled by dropping .twilio-functions files in favor of a dedicated config file and a deploy info cache file. BREAKING CHANGE: Drops support for .twilio-functions files and internally restructures activate files to promote fix #166
96a6c4d
to
1e17858
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
This PR aims to restructure the way configuration is handled by dropping .twilio-functions files in
favor of a dedicated config file and a deploy info cache file.
BREAKING CHANGE: Drops support for .twilio-functions files and internally restructures activate
files to promote
fix #166
Contributing to Twilio