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

Clean up godoc representation of packages and document code gen tool #35

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kortschak
Copy link

@kortschak kortschak commented Jul 24, 2021

Please take a look.

To demonstrate what is being fixed, visit https://pkg.go.dev/github.com/elastic/[email protected]/gotype. This also makes generation consistent by including the generator in the mod files and fixes imports.

It looks like the code gen does not automatically place the copyright headers. If you point me to where this is done I will fix that. Thanks. Fixed

@elasticmachine
Copy link
Collaborator

elasticmachine commented Jul 24, 2021

❕ Build Aborted

The PR is not allowed to run in the CI yet

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Start Time: 2023-08-16T15:02:14.291+0000

  • Duration: 2 min 58 sec

Steps errors 2

Expand to view the steps failures

Load a resource file from a library
  • Took 0 min 0 sec . View more details here
  • Description: approval-list/elastic/go-structform.yml
Error signal
  • Took 0 min 0 sec . View more details here
  • Description: githubApiCall: The REST API call https://api.github.com/orgs/elastic/members/kortschak return the message : java.lang.Exception: httpRequest: Failure connecting to the service https://api.github.com/orgs/elastic/members/kortschak : httpRequest: Failure connecting to the service https://api.github.com/orgs/elastic/members/kortschak : Code: 404Error: {"message":"User does not exist or is not a member of the organization","documentation_url":"https://docs.github.com/rest/orgs/members#check-organization-membership-for-a-user"}

@kortschak kortschak force-pushed the doc/clean branch 2 times, most recently from 6089fc8 to 7692c18 Compare July 24, 2021 01:57
github.com/urso/mktmpl v0.0.0-20180112143305-7705185786e8 // indirect
golang.org/x/tools v0.1.5 // indirect
gopkg.in/yaml.v2 v2.4.0 // indirect
)
Copy link

Choose a reason for hiding this comment

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

I presume most of these dependencies are introduced by the mktmpl tool.

For tools I would prefer to bingo approach instead of 'polluting' the go.mod file.

Copy link
Author

@kortschak kortschak Aug 9, 2021

Choose a reason for hiding this comment

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

Yes, that's why they're there. I'm not convinced by the claims made by bingo, but is there anything that you would like me to do with this? Either I should add a tools.go to make the changes to go.mod stable, or I can try bingo.

Copy link

Choose a reason for hiding this comment

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

Let's add a tools.go to stabilize go.mod. Thanks

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@urso
Copy link

urso commented Aug 9, 2021

All in all the change looks good to me. Thanks for contributing clean ups. Very much appreciated.

I'm just not a fan of polluting go.mod with tool dependencies. Maybe we want to give 'bingo' a try.
Besides for some benchmarks (which are disabled by default), testify should be the only dependency.


package gotype

import _ "github.com/urso/mktmpl"

Choose a reason for hiding this comment

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

a blank import should be only in a main or test package, or have a comment justifying it


package gotype

import _ "github.com/urso/mktmpl"

Choose a reason for hiding this comment

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

a blank import should be only in a main or test package, or have a comment justifying it

@urso
Copy link

urso commented Aug 10, 2021

/test

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.

4 participants