-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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: add startswith and endswith funcs #31220
feat: add startswith and endswith funcs #31220
Conversation
Thanks for this submission! It is in the queue to be reviewed. Thanks again! |
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 is great, thanks for the contribution! This behaviour makes sense to me and I checked it out locally.
I requested a few additional test cases inline. We'll also need documentation for these new functions, which means:
- Adding a new page to the
language/functions
directory, with the same format as other string functions - Adding the page to the
language-nav-data.json
file - Updating the
layouts/language.erb
template
I'm adding our Technical Writer as a reviewer on this PR, who should be able to help review the docs once you've written them.
Hi there! I'm excited to see this new functionality, and thank you to @alisdair for pointing out the need for docs! :) I think a great approach to maintain the format we use for these pages is to copy and paste the .mdx from an existing string function page and then modify it for the new function that you're adding. I would love to review your docs changes once you've drafted them. If you have questions at any point in the process, please just leave a comment in this PR and tag me, and I'd be happy to help. Thank you again for your contributions! |
@alisdair Unsure what broke with Vercel. Let me know if there's anything I need to do. @laurapacilio Please feel free tear apart my first run at documentation 😄 |
f927cf3
to
f74e93a
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.
Thanks! I'll leave the docs review for Laura, but the code is ✅ from me.
I think the Vercel failure is because we don't have entries in the language-nav-data.json file at the top level of the functions
part of the tree. Other functions have an entry for their virtual sub-section (which you've added) and a separate hidden
entry at the functions
level, later in the file. For example:
terraform/website/data/language-nav-data.json
Lines 861 to 862 in f74e93a
{ "title": "split", "path": "functions/split", "hidden": true }, | |
{ "title": "strrev", "path": "functions/strrev", "hidden": true }, |
f74e93a
to
2278d30
Compare
That was it! Thank you! |
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.
A couple of minor edits for you to consider - thank you for your great work on this! :)
My goal with the edits is to streamline the description a bit (I think we likely don't need to use "given" after we initially say that the function takes two values) and clarify the types of values. Let me know if I've misunderstood the way these functions work or you have other feedback. Thank you!
Co-authored-by: Laura Pacilio <83350965+laurapacilio@users.noreply.github.com>
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.
Approving because I think this is great! Just a few final minor wording nits to remove an extra word. Would you mind merging those in as well?
Otherwise, @alisdair I think this is ready - I'd like you to do the honors, since this contains code as well as docs :)
I like it! I went with language that was similar to another set of docs, but this is clearer. Thank you! |
Co-authored-by: Laura Pacilio <83350965+laurapacilio@users.noreply.github.com>
Thanks again! 🎉 |
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Purpose: Add
startswith
andendswith
built-in functions to TerraformPer the issue referenced below, these are functions that the Terraform team is willing to accept due to the broad use case and interest. I have not submitted to the
terraform
repo before, so my apologies if I missed any conventions or standards. I tried to keep it as close to the other code that I saw as possible. If there's anything that should be changed please let me know.Referencing Issue: #28209