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

Anchors #409

Merged
merged 1 commit into from
Mar 2, 2021
Merged

Anchors #409

merged 1 commit into from
Mar 2, 2021

Conversation

nitrocode
Copy link
Contributor

@nitrocode nitrocode commented Feb 26, 2021

Description of your changes

Fixes #408

I have:

How has this code been tested

Tested this locally and seems to work well with a single test case. Not all test cases pass. Waiting for more discussion.

Is there a way to regenerate the testdata or should I be updating that manually ?

Copy link
Member

@khos2ow khos2ow left a comment

Choose a reason for hiding this comment

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

I really like the idea @nitrocode and just wanted to give you a very early feedback.

internal/format/markdown_table.go Outdated Show resolved Hide resolved
@khos2ow
Copy link
Member

khos2ow commented Feb 26, 2021

Please hold off any major work on testing until #410 is done.

@nitrocode
Copy link
Contributor Author

Ah I just added the --anchors switch and pushed it. I will hold off on making additional changes.

@khos2ow khos2ow mentioned this pull request Mar 1, 2021
2 tasks
Copy link
Member

@khos2ow khos2ow left a comment

Choose a reason for hiding this comment

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

Thank you @nitrocode for starting working on this. This looks ok so far but it needs a bit of changing.

Also the commits in the PR should not contain a "merge commit" and they always need to be rebased. But it's completely up to you to have that in mind along the way, or just squash everything at the very end.

internal/template/template.go Outdated Show resolved Hide resolved
internal/template/anchor.go Outdated Show resolved Hide resolved
cmd/markdown/markdown.go Outdated Show resolved Hide resolved
@nitrocode
Copy link
Contributor Author

I'm having issues running test on the master branch.

$ make test
package github.com/nitrocode/terraform-docs/scripts/docs
	scripts/docs/generate.go:28:2: use of internal package github.com/terraform-docs/terraform-docs/internal/terraform not allowed

@khos2ow
Copy link
Member

khos2ow commented Mar 1, 2021

I'm wondering why it says package github.com/nitrocode/terraform-docs/scripts/docs? Technically you cannot access internal packages from github.com/terraform-docs/terraform-docs from github.com/nitrocode/terraform-docs.

Are you in GOPATH? If yes, $GOPATH/src/github.com/terraform-docs/terraform-docs should be your local cloned folder.

@nitrocode
Copy link
Contributor Author

Thanks. I upgraded golang outside of homebrew and it's working now.

@nitrocode nitrocode force-pushed the anchors branch 10 times, most recently from 99ae02b to 9e8cd7a Compare March 2, 2021 13:59
@nitrocode
Copy link
Contributor Author

I was able to fix the commit history by basically deleting my branch and updating the files again... squashing commits before and after an in-between merge does not seem possible.

The asciidoc template doesn't look like it will work currently as anchors are different in asciidoc vs markdown.

Docs

[[anchor-1]]
Paragraph or block 1.

anchor:anchor-2[]
Paragraph or block 2.

<<anchor-1>>,
<<anchor-1,First anchor>>,
xref:anchor-2[],
xref:anchor-2[Second anchor].

I'll use the above model to update the asciidoc anchors.

@nitrocode nitrocode force-pushed the anchors branch 3 times, most recently from c6b5cdd to b7fc567 Compare March 2, 2021 14:38
@nitrocode
Copy link
Contributor Author

nitrocode commented Mar 2, 2021

@khos2ow could you review this again please ?

I'm unsure how to resolve the 2 errors below.

  • codecov < 75%
  • Generating docs throws an error and I see this on the master branch
    ✗ GOOS=linux GOARCH=amd64 go1.16 run ./scripts/docs/generate.go
    fork/exec /var/folders/2v/qj0vxglj3hsgcs5m_f4jprnw0000gn/T/go-build361031716/b001/exe/generate: exec format error
    

@nitrocode nitrocode force-pushed the anchors branch 5 times, most recently from cd7820e to a4f3a48 Compare March 2, 2021 15:04
@nitrocode nitrocode requested a review from khos2ow March 2, 2021 15:05
Copy link
Member

@khos2ow khos2ow left a comment

Choose a reason for hiding this comment

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

Couple of minor comments.

cmd/asciidoc/asciidoc.go Outdated Show resolved Hide resolved
internal/template/anchor_test.go Outdated Show resolved Hide resolved
@khos2ow
Copy link
Member

khos2ow commented Mar 2, 2021

  • codecov coverage dropped by 0.29% which can be ignored, but you also can add cases into templates_test.go#TestBuiltinFunc to cover newly added functions too.
  • that's weird. what does make docs do, same error? and why do you have GOOS=linux? I thought you were using macos.

@nitrocode
Copy link
Contributor Author

Thanks for reviewing. I'll jump on it today.

Yes, I'm on OSX and the issue was related to the go1.16 issue again...

Not to add too much scope creep to this PR but it would be nice to use an env var in the Makefile like GOBINARY=go which can be used in every target then if I need to overwrite it, I can simply do either

export GOBINARY=go1.16
make docs

or

GOBINARY=go1.16 make docs

@nitrocode nitrocode force-pushed the anchors branch 2 times, most recently from a1d4200 to 1f5e439 Compare March 2, 2021 17:22
@khos2ow
Copy link
Member

khos2ow commented Mar 2, 2021

That's a really good point, I'll add it in another PR! 👍

@nitrocode nitrocode requested a review from khos2ow March 2, 2021 17:31
Copy link
Member

@khos2ow khos2ow left a comment

Choose a reason for hiding this comment

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

One last thing to do, please add anchor in:

  • internal/cli/reader.go#L96
  • docs/reference/config-file.md

Also please add a bit more information to commit message, as it will be shown in release note.

Signed-off-by: nitrocode <nitrocode@users.noreply.github.com>
Copy link
Member

@khos2ow khos2ow left a comment

Choose a reason for hiding this comment

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

Thank you @nitrocode for all the efforts on this PR! 👍

@khos2ow khos2ow merged commit d1fea03 into terraform-docs:master Mar 2, 2021
@nitrocode nitrocode deleted the anchors branch March 2, 2021 17:50
@nitrocode
Copy link
Contributor Author

I looked at all the tickets for the v0.12 project and it seems like this won't be released for some time. Would you be willing to cut a terraform-docs v0.12-beta1 release like they do with terraform ?

@khos2ow
Copy link
Member

khos2ow commented Mar 2, 2021

Yeah sure I'll cut a release soon, but also in the meantime you can use edge docker image, which is always HEAD of master. (although that might force a change in your workflow, but at least it's an option to immediately unblock you)

@nitrocode
Copy link
Contributor Author

@khos2ow I'm playing with the new release now and it works great!

One thing. On the release notes, you put @ 9trocode instead of @nitrocode 😅

@khos2ow
Copy link
Member

khos2ow commented Mar 23, 2021

Interesting! The list of contributors are being extracted automatically, I should check that script to see why that happened! 😅

Updated release note, thanks for pointing this out and your contribution!

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.

Anchor support for resources, modules, inputs, outputs, etc
2 participants