-
Notifications
You must be signed in to change notification settings - Fork 853
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
Updated tools/generator
to read from literate files
#793
Conversation
The way we specified `go build` enumerated each package to be compiled. This created a lot of noise because folders that didn't contain any Go source themselves, but had children that did caused an error message. This way, we'll still get all of the protection of making sure that our entire repo builds, without the noise. Also, a couple of formatting changes in profileBuilder.
Also, further documentation improvements
Alright @jhendrixMSFT / @mcardosos: This PR is ready for feedback and merge. It is no longer a WIP |
tools/profileBuilder/latest.go
Outdated
currentGroup := operationGroup{ | ||
provider: pathMatches[1], | ||
resourceType: pathMatches[2], | ||
group: pathMatches[3], | ||
resourceType: pathMatches[4], |
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.
resourceType: pathMatches[4], [](start = 4, length = 29)
does this also work with services that have a parentResourcePath
? What about graphrbac? Their paths are not like the rest of ARM
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.
Yep, I've been adding in support for broader and broader sets of Swaggers. The regexp itself is structured to take care of those differences at the moment.
return | ||
} | ||
// version should be set by the linker when compiling this program by providing the following arguments: | ||
// -X main.version=<version> |
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.
[](start = 19, length = 9)
Here do you mean the semantic version? or the git commit?
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.
I've added more clarifying lines later locally.
Though to answer the immediate question, the git commit :)
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.
Remind me plz, where do you use the git commit?
tools/generator/program.go
Outdated
return regexp.MustCompile(strings.Join(elements, `[/\\]`)) | ||
// getDefaultOutputBase returns the default location of the Azure-SDK-for-Go on your filesystem. | ||
func getDefaultOutputBase() string { | ||
return strings.Replace(path.Join(goPath(), "src", "github.com", "Azure", "azure-sdk-for-go"), `\`, "/", -1) |
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.
normalizePath
:D ?
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.
Derp, yes good catch! Fixed locally
tools/generator/program.go
Outdated
if output == nil { | ||
output = ioutil.Discard | ||
} | ||
targetFiles = collection.AsEnumerable(flag.Args()) |
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.
Args [](start = 44, length = 4)
Example?
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.
These are the arguments to the program. https://godoc.org/flag#Args
Is that what you mean?
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.
I mean an example of a program argument, what can I input as argument?
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.
I just added a new commit which replaces the flag.Usage
global variable function, the new tells the caller that the expected arguments are literate files.
|
||
// This function is a collection.Unfolder which takes a literate file as a path, and retrieves all configuration which applies to a package tag and Go. | ||
return func(subject interface{}) collection.Enumerator { | ||
results := make(chan interface{}) |
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.
can it be a generationTuple
from the beginning?
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.
I'm afraid not :/
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.
That's not to say there isn't anything we could do to make it nicer! But the generationTuple type would need to be significantly modified.
tools/generator/program.go
Outdated
fmt.Println("Cloning Azure REST API Specs to: ", cloneLoc) | ||
fmt.Println("Execution Time: ", time.Now().Sub(start)) | ||
fmt.Println("Generated: ", generatedCount) | ||
fmt.Println("Formatted: ", formattedCount) |
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.
golint
and go vet
are useful too :D
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.
Fair enough!
debugLog *log.Logger | ||
version *semver.Version | ||
targetFiles collection.Enumerable | ||
packageVersion string |
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.
packageVersion [](start = 1, length = 14)
Where is this set?
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.
Fixed locally.
To take a step back, how come we need our own generation program? With all the language-specific generation options in the config files it seems that we should only need one tool to perform generation for any language. |
BTW, how are you whitelisting dataplane stuff? |
I already discussed all of the following with @mcardosos and @jhendrixMSFT offline, but I wanted to copy this over for community involvement.
Agreed, ideally we wouldn't have to have such a tool. However, at the moment it's the best thing we've got. I'm in talks with @johanste, @lmazuel, and @sergey-shandar about how we could extend our current tool chain to have some reusable means of generating SDKs from README tags.
Specifically, having a |
@mcardosos / @jhendrixMSFT any final thoughts before I merge this in? |
Not many, not many |
Previously, separate files in the profileBuilder had different build tags. This would cause errors when package `main` didn't have a `main` function.
This is the generator for the next generation package in our SDK that provide multi-API Version and profile support.