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

fix(template functions): Use function Title instead of ToTitle #165

Closed
wants to merge 1 commit into from

Conversation

anmoel
Copy link
Contributor

@anmoel anmoel commented Jun 30, 2022

Use function Title instead of ToTitle to get capitalize strings instead of upper strings

Using cases.Title because the strings.Title function is deprecated and refs to cases.Title

resolves #164

@anmoel anmoel requested a review from a team as a code owner June 30, 2022 14:12
@hashicorp-cla
Copy link

hashicorp-cla commented Jun 30, 2022

CLA assistant check
All committers have signed the CLA.

@anmoel anmoel force-pushed the fix/template-title branch 2 times, most recently from 2178224 to da9bcc7 Compare June 30, 2022 14:24
@detro
Copy link
Contributor

detro commented Jun 30, 2022

Hey @anmoel - thanks for taking the time to look into this.

I think you are right (ToString() is probably not the right choice here), but your implementation I don't believe it would work.

You need to initialize a caser, doing something like:

package main

import (
	"fmt"
	"strings"

	"golang.org/x/text/cases"
	"golang.org/x/text/language"
)

func main() {
	str := "my Odly cAsed striNg"
	caser := cases.Title(language.Und)

	fmt.Println("ToLower: ", strings.ToLower(str))
	fmt.Println("ToUpper: ", strings.ToUpper(str))
	fmt.Println("ToTitle: ", strings.ToTitle(str))
	fmt.Println("Title: ", strings.Title(str))
	fmt.Println("Caser: ", caser.String(str))
}

@anmoel
Copy link
Contributor Author

anmoel commented Jun 30, 2022

I will add a test in my branch and will check this

@anmoel
Copy link
Contributor Author

anmoel commented Jul 1, 2022

Hi @detro,

i fixed the code and added tests for some template functions.
is there a reason why you use go cmp instead of testify/assert to compare objects in tests?

Copy link
Contributor

@detro detro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, thanks for this.

A few things to address though:

  • removal of testify
  • update of the CHANGELOG to add a new bugfix entry
  • version should be v0.12.1
  • update of the README to point at which function, with which casing, is used for the title template function

import (
"testing"

"github.com/stretchr/testify/assert"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a long standing position of HC to try to avoid dependency that are not required.

Please use go-cmp that is already part of the codebase for these tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is done

… Capitalize strings instead of upper strings

using cases.Title because the strings.Title function is deprecated and refs to cases.Title
"github.com/google/go-cmp/cmp"
)

func TestRenderStringTemplate(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks so much for those tests. Really appreciated.

Comment on lines +171 to +174
| `title` | Equivalent to [`cases.Title`](https://pkg.go.dev/golang.org/x/text/cases#Title). |
| `tffile` | A special case of the `codefile` function, designed for Terraform files (i.e. `.tf`). |
| `trimspace` | Equivalent to [`strings.TrimSpace`](https://pkg.go.dev/strings#TrimSpace). |
| `upper` | Equivalent to [`strings.ToLower`](https://pkg.go.dev/strings#ToUpper). |
| `upper` | Equivalent to [`strings.ToUpper`](https://pkg.go.dev/strings#ToUpper). |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well spotted!

Comment on lines +80 to +81
* cmd/tfplugindocs: Use existing Terraform CLI binary if available on PATH, otherwise download latest Terraform CLI binary (<https://github.com/hashicorp/terraform-plugin-docs/pull/124>).
* cmd/tfplugindocs: Added `tf-version` flag for specifying Terraform CLI binary version to download, superseding the PATH lookup (<https://github.com/hashicorp/terraform-plugin-docs/pull/124>).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* cmd/tfplugindocs: Use existing Terraform CLI binary if available on PATH, otherwise download latest Terraform CLI binary (<https://github.com/hashicorp/terraform-plugin-docs/pull/124>).
* cmd/tfplugindocs: Added `tf-version` flag for specifying Terraform CLI binary version to download, superseding the PATH lookup (<https://github.com/hashicorp/terraform-plugin-docs/pull/124>).
* cmd/tfplugindocs: Use existing Terraform CLI binary if available on PATH, otherwise download latest Terraform CLI binary ([#124](https://github.com/hashicorp/terraform-plugin-docs/pull/124)).
* cmd/tfplugindocs: Added `tf-version` flag for specifying Terraform CLI binary version to download, superseding the PATH lookup ([#124](https://github.com/hashicorp/terraform-plugin-docs/pull/124)).

Comment on lines +85 to +86
* cmd/tfplugindocs: Swapped `.Type` and `.Name` resource and data source template fields so they correctly align (<https://github.com/hashicorp/terraform-plugin-docs/pull/44>).
* schemamd: Switched attribute name rendering from bold text to code blocks so the Terraform Registry treats them as anchor links (<https://github.com/hashicorp/terraform-plugin-docs/pull/59>).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* cmd/tfplugindocs: Swapped `.Type` and `.Name` resource and data source template fields so they correctly align (<https://github.com/hashicorp/terraform-plugin-docs/pull/44>).
* schemamd: Switched attribute name rendering from bold text to code blocks so the Terraform Registry treats them as anchor links (<https://github.com/hashicorp/terraform-plugin-docs/pull/59>).
* cmd/tfplugindocs: Swapped `.Type` and `.Name` resource and data source template fields so they correctly align ([#44](https://github.com/hashicorp/terraform-plugin-docs/pull/44)).
* schemamd: Switched attribute name rendering from bold text to code blocks so the Terraform Registry treats them as anchor links ([#59](https://github.com/hashicorp/terraform-plugin-docs/pull/59)).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for spotting those, but the format should be [#NUM](URL_to_NUM) (as you correctly used above)

Comment on lines +1 to +6
# 0.12.1 (July 04, 2022)

BUG FIXES:

* template functions: funtion `title` creates capitalize strings instead of upper strings ([#165](https://github.com/hashicorp/terraform-plugin-docs/pull/165)).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another PR landed so I think you need to rebase and put this in the 0.13.0 section, but still under BUG FIXES.

Copy link
Contributor

@detro detro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments and suggestions/corrections, but I think we are pretty much there.

It needs rebasing though.

@detro
Copy link
Contributor

detro commented Jul 6, 2022

@anmoel I tried to do the rebasing locally, and it's really just about the CHANGELOG.

Once you rebase, we can merge and then I'll start the release process.

@detro
Copy link
Contributor

detro commented Jul 7, 2022

Hi @anmoel - as I'd like to fix the bug, I'm happy to take it upon myself to import your commit locally, do the tweaks and rebasing, and open a new PR that superseeds this one.

If you can't get to it today, I'll try to do this either tomorrow or Monday at the latest.

@detro
Copy link
Contributor

detro commented Jul 8, 2022

Superseeded by #168

Thank you so much for your fix: I have made sure your commit was correctly adjudicated.

@detro detro closed this Jul 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Template function "title" creates a "UPPER" string instead of "Title"
3 participants