- Notifications
You must be signed in to change notification settings - Fork1k
feat(site): add support for.sh and.tpl files to template editor#9674
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
.sh and.tpl files to template editorThere 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.
suggested some changes that feel a bit cleaner to me
| }; | ||
| constlanguageByExtension:Record<string,string>={ | ||
| constlanguageByExtension:Record<AllowedExtension,string>={ |
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.
| constlanguageByExtension:Record<AllowedExtension,string>={ | |
| constlanguageByExtension={ |
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.
actually, move this wholeconst to templateVersion.ts
BrunoQuaresmaSep 13, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Since this is only used on TemplateFiles, I would keep it there.
PS: After seeing your other comment I think it would make sense if thelanguageByExtesion would define the types but I left a comment there why I don't think this is the best option.
| exportconstallowedExtensions=[ | ||
| "tf", | ||
| "md", | ||
| "mkd", | ||
| "Dockerfile", | ||
| "protobuf", | ||
| "sh", | ||
| "tpl", | ||
| ]asconst; | ||
| exporttypeAllowedExtension=(typeofallowedExtensions)[number]; |
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.
| exportconstallowedExtensions=[ | |
| "tf", | |
| "md", | |
| "mkd", | |
| "Dockerfile", | |
| "protobuf", | |
| "sh", | |
| "tpl", | |
| ]asconst; | |
| exporttypeAllowedExtension=(typeofallowedExtensions)[number]; | |
| exporttypeAllowedExtension=keyoftypeoflanguageByExtension; |
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.
Hm... in this case theAllowedExtension should be defined from the arrayallowedExtensions and not the other way around. By having it in this way, we are not going to forget to update thelanguageByExtension when a new extension is added.
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.
we just aren't using theallowedExtensions variable anywhere. we could even just definetype AllowedExtension = 'tf' | 'md' | ...
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.
we are! we use it to do some checks + filtering when unzipping the template files.
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.
oh, well then nevermind lmao. you're doing great. 💕
@aslilac I'm going to merge this now but we can keep the discussion moving forward |
Close#7921