Skip to content
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

Merged
merged 5 commits into from Jul 14, 2022

Conversation

FooBartn
Copy link
Contributor

@FooBartn FooBartn commented Jun 9, 2022

Purpose: Add startswith and endswith built-in functions to Terraform

Per 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

@hashicorp-cla
Copy link

hashicorp-cla commented Jun 9, 2022

CLA assistant check
All committers have signed the CLA.

@FooBartn FooBartn marked this pull request as ready for review June 9, 2022 22:08
@alisdair alisdair requested a review from a team June 10, 2022 15:38
@crw
Copy link
Collaborator

crw commented Jun 10, 2022

Thanks for this submission! It is in the queue to be reviewed. Thanks again!

Copy link
Member

@alisdair alisdair left a 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:

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.

internal/lang/functions_test.go Show resolved Hide resolved
@laurapacilio
Copy link
Contributor

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!

@FooBartn
Copy link
Contributor Author

@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 😄

Copy link
Member

@alisdair alisdair left a 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:

{ "title": "split", "path": "functions/split", "hidden": true },
{ "title": "strrev", "path": "functions/strrev", "hidden": true },

@FooBartn
Copy link
Contributor Author

FooBartn commented Jul 6, 2022

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:

That was it! Thank you!

@alisdair alisdair requested a review from a team July 7, 2022 15:34
Copy link
Contributor

@laurapacilio laurapacilio left a 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!

website/docs/language/functions/endswith.mdx Outdated Show resolved Hide resolved
website/docs/language/functions/endswith.mdx Outdated Show resolved Hide resolved
website/docs/language/functions/endswith.mdx Outdated Show resolved Hide resolved
website/docs/language/functions/startswith.mdx Outdated Show resolved Hide resolved
website/docs/language/functions/startswith.mdx Outdated Show resolved Hide resolved
website/docs/language/functions/startswith.mdx Outdated Show resolved Hide resolved
Co-authored-by: Laura Pacilio <83350965+laurapacilio@users.noreply.github.com>
Copy link
Contributor

@laurapacilio laurapacilio left a 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 :)

website/docs/language/functions/startswith.mdx Outdated Show resolved Hide resolved
website/docs/language/functions/endswith.mdx Outdated Show resolved Hide resolved
website/docs/language/functions/endswith.mdx Outdated Show resolved Hide resolved
@FooBartn
Copy link
Contributor Author

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>
@alisdair alisdair merged commit d3284bd into hashicorp:main Jul 14, 2022
@alisdair
Copy link
Member

Thanks again! 🎉

@github-actions
Copy link

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

@github-actions
Copy link

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants