bring back enum generation #158

sclasen · 2015-03-31T20:06:22Z

add back the enum template, and restore the needed funcs for massaging the enums from the schemata.

sclasen · 2015-03-31T20:06:37Z

Fixes #155

sclasen · 2015-03-31T20:53:02Z

@lsegal using the ExportableName method yields some enums that are not go style guide approved, is this ok? I think the exportable func is more stylistically correct?

For instance

-       ConfigurationItemStatusOK         = "Ok"
+       ConfigurationItemStatusOk         = "Ok"

-       ResourceTypeAWSEC2VPNconnection    = "AWS::EC2::VPNConnection"
-       ResourceTypeAWSEC2VPNgateway       = "AWS::EC2::VPNGateway"
        ResourceTypeAWSEC2Volume           = "AWS::EC2::Volume"
+       ResourceTypeAWSEC2Vpnconnection    = "AWS::EC2::VPNConnection"
+       ResourceTypeAWSEC2Vpngateway       = "AWS::EC2::VPNGateway"

sclasen · 2015-03-31T21:32:48Z

@lsegal got rid of the map. Still seems to be an issue with ExportableName though. any advice?

sclasen · 2015-03-31T21:36:45Z

service/configservice/api.go

+	ResourceTypeAWSEC2VPC              = "AWS::EC2::VPC"
+	ResourceTypeAWSEC2Volume           = "AWS::EC2::Volume"
+	ResourceTypeAWSEC2Vpnconnection    = "AWS::EC2::VPNConnection"
+	ResourceTypeAWSEC2Vpngateway       = "AWS::EC2::VPNGateway"


@lsegal here is a bad one.

lsegal · 2015-03-31T21:39:16Z

@sclasen ExportableName is used to generate all shape and operation names, so if there is a problem there it is likely affecting other generated names and we should resolve that there. Note that ExportableName needs to be used in the load passes of the API, not during template generation, in order to generate the proper errors when inflections fail.

sclasen · 2015-03-31T21:44:32Z

sclasen#1 shows the diff of using the original exportable function instead of ExportableName, which results in much better code imo.

lsegal · 2015-03-31T21:48:01Z

@sclasen only for those very specific examples, and it doesn't work in all cases. In order to guarantee consistency across all exported names going forward we need to explicitly whitelist all words (see inflections.csv) we will have to rely on ExportableName.

lsegal · 2015-03-31T21:52:45Z

@sclasen basically the ExportableName function is made to be a more consistent replacement system. If it's not working correctly we need to fix that rather than abandoning it for enums only. The diff showing incorrectly inflected terms like "VPN" lead me to believe that it's not being setup properly, because these terms are being correctly inflected for all other operation / shape names.

sclasen · 2015-03-31T22:16:08Z

@lsegal do you mean not set up properly for enums alone somehow or in general not set up correctly?

is there a setup func that one can call on shape.API?

lsegal · 2015-03-31T22:29:30Z

@sclasen given that it works on other shapes, it's probably not setup correctly for enums. I mentioned above how to use the load passes to correctly setup enums while loading instead of while generating template output-- it doesn't look like this is being done.

sclasen · 2015-03-31T22:38:00Z

Aha missed that comment. wired it in passes instead, and make complains about a ton of missing inflections, I added them and things are looking better.

lsegal · 2015-03-31T22:41:27Z

Looks good, but we probably need to go through those inflections and make sure they are correct. I see a few that need some changes.

sclasen · 2015-03-31T22:41:53Z

@lsegal looking better, though now I am noticing that the generator is picking up older schemas, for example cloudfront has 2 schemas. Is there an incantation to make the generator pick the later schema?

lsegal · 2015-03-31T22:45:35Z

@sclasen you can explicitly call go run ./internal/model/cli/gen-api/main.go apis/rds/rds-YYYY-MM-DD.normal.json to call for that specific version.

sclasen · 2015-03-31T22:46:35Z

@lsegal ok, I deleted the older cloudfront schemas and ran the generator and it is of course correct. Should older shcemas be deleted?

lsegal · 2015-03-31T22:47:21Z

@sclasen no, old schemas are supported and may be generated in the future under different packages.

sclasen · 2015-03-31T22:53:00Z

Alright, is there a mechanism for using only the latest generator other than explicitly enumerating the go:generate calls in service/generate.go ?

sclasen · 2015-03-31T22:57:58Z

moved the old ones to the side for generation for now

lsegal · 2015-03-31T22:58:14Z

is there a mechanism for using only the latest generator other than explicitly enumerating the go:generate calls in service/generate.go ?

@sclasen this is not yet supported. As mentioned in the comments, the SDK should be generating with the latest API version, but there might be another (unrelated) generator bug here.

sclasen · 2015-03-31T23:00:57Z

internal/model/api/inflections.csv

+Between:
+Eip:EIP
+Vpnconnection:VPNconnection
+Vpngateway:VPNgateway


Assume these 2 should be VPNConnection and VPNGateway @lsegal ?

sclasen · 2015-03-31T23:38:39Z

@lsegal deduped inflections, were a ton! let me know if anything else is needed.

sclasen · 2015-04-01T00:32:56Z

squashed commits

danp · 2015-04-01T01:08:03Z

👏

…that build complained are missing.

sclasen · 2015-04-02T16:06:05Z

@lsegal anything needed here? we have a pile of code that uses the pre develop-merge code, and we use quite a ton of the consts, so we cant switch over till this goes in.

lsegal · 2015-04-06T21:15:56Z

@sclasen looking through the generated inflections, I'm still seeing a lot of missing or incorrect entries. Ideally all country codes, for example, should be capitalized. We'll still need some time to look through the code in this PR and make sure it's generating the right things.

I think there's also still a general question about how these enums will be exposed and what kind of possible bloat will they be providing. Some of this discussion may change how the generation is done. Right now, the generated constants have no association to the structure types they are being used inside of, which makes for a poor documentation story. One thing that could alleviate this would be to create special string alias types for these constants, which would give better visibility to where these constants are used. Another thing I noticed is that the type prefix on these constants occasionally makes for very long names, which could potentially be resolved by relying on the type alias for communicating part of that name. Finally, I'm concerned about some constants bloating the package, specifically the country codes in Route53 Domains, which seems a little superfluous to me. Having a list of 100+ constants in documentation might be distracting and not really provide that much value.

Again, I'm not opposed to constants, but I'd like to see if we can address some of these points before merging.

In the meantime, while we look at the inflections and think about how to improve visibility in documentation, you should be able to generate these constant names in a separate package in your own codebase (i.e. copy paste the string consts you use) and import that for now. That should unblock you until this gets merged in while still allowing you to switch over the rest of your code.

sclasen · 2015-04-06T21:57:10Z

Thanks @lsegal

fwiw I have found the type prefixes on the constants pretty helpful, especially from an autocomplete perspective, you type swf.EventType<autocomplete keystroke> and get a dropdown of all the correct consts.

My fear is that if we use type aliasing instead of prefixes, the tooling will have to be much smarter to give you a reasonable dropdown. Long names are not a problem with tooling.

Do you think adding a enum generation blacklist mechanism would help? Unhelpful enums such as route53domains country code could simply be skipped. Let me know if you'd like me to add that feature.

sclasen · 2015-04-11T01:16:26Z

@lsegal another question here, but more general.

Is it sane to consider building other (external to this project) generators, based on internal/model/api/API.go

If it were fairly well documented/stable it would be pretty easy to show folks how to generate arbitrary code that you dont necessarily want in this project. For instance

package main

import(
    "github.com/awslabs/aws-sdk-go/internal/model/api"
    "os"
    "fmt"
)


func main(){
    service := "swf"
    date := "2012-01-25"
    gopath := os.Getenv("GOPATH")
    api := new(api.API)
    api.Attach(fmt.Sprintf("%s/src/github.com/awslabs/aws-sdk-go/apis/%s/%s.normal.json", gopath, service, date))
    for _, o := range api.ShapeList(){
        if len(o.Enum) > 0 {
            for _, e := range o.Enum {
                fmt.Printf("%s->%s\n", o.ShapeName, e)
            }
        }
    }
}

danp · 2015-05-08T17:45:40Z

@lsegal any thoughts on @sclasen's responses? Would be great to get some movement on this. Looks like it needs a rebase as well.

lsegal · 2015-05-12T22:32:17Z

@dpiddy @sclasen we're still looking at this. I think the best way to move forward is to use the generated enums in an external (vendored) package so you are not blocked by this pull request. Specifically, given the simplicity of the enums themselves, you don't even need to rely on the internal logic (i.e. #201) to parse the JSON models and pull out enum structures into Go code.

lsegal · 2015-05-12T23:01:58Z

For example, here's a playground link for parsing enums out of an abridged version of the autoscaling/2011-01-01.normal.json file: http://play.golang.org/p/JLHAHwOusH

It would be fairly simple to convert this into a generalized executable to write enums for various service packages.

Note that this change disables inflections for all enum constants. Resolves #158 Fixes #155

References: #124, #155, #157, #158, #276, #308

sclasen reviewed Mar 31, 2015
View reviewed changes

sclasen force-pushed the enums branch from ca3c778 to 90861eb Compare April 1, 2015 00:32

sclasen added 2 commits April 1, 2015 12:25

bring back enum generation, gen services with enums. add inflections …

be57ccc

…that build complained are missing.

regen after rebase

d269646

sclasen force-pushed the enums branch from 90861eb to d269646 Compare April 1, 2015 19:28

sclasen mentioned this pull request Apr 26, 2015

[discussion] Is using the code in internal/model/api 'supported' ? #201

Closed

jasdel mentioned this pull request May 21, 2015

Leaky abstraction around HTTP API #218

Closed

lsegal added a commit that referenced this pull request Jul 16, 2015

Add enums

4e49ff9

Note that this change disables inflections for all enum constants. Resolves #158 Fixes #155

lsegal mentioned this pull request Jul 16, 2015

Add enum generation #317

Closed

lsegal closed this in b311b2e Jul 28, 2015

jasdel added a commit that referenced this pull request Jul 28, 2015

Tag release v0.7.0

67dc9f9

References: #124, #155, #157, #158, #276, #308

bring back enum generation #158

bring back enum generation #158

Uh oh!

Conversation

sclasen commented Mar 31, 2015

Uh oh!

sclasen commented Mar 31, 2015

Uh oh!

sclasen commented Mar 31, 2015

Uh oh!

sclasen commented Mar 31, 2015

Uh oh!

sclasen Mar 31, 2015

Choose a reason for hiding this comment

Uh oh!

lsegal commented Mar 31, 2015

Uh oh!

sclasen commented Mar 31, 2015

Uh oh!

lsegal commented Mar 31, 2015

Uh oh!

lsegal commented Mar 31, 2015

Uh oh!

sclasen commented Mar 31, 2015

Uh oh!

lsegal commented Mar 31, 2015

Uh oh!

sclasen commented Mar 31, 2015

Uh oh!

lsegal commented Mar 31, 2015

Uh oh!

sclasen commented Mar 31, 2015

Uh oh!

lsegal commented Mar 31, 2015

Uh oh!

sclasen commented Mar 31, 2015

Uh oh!

lsegal commented Mar 31, 2015

Uh oh!

sclasen commented Mar 31, 2015

Uh oh!

sclasen commented Mar 31, 2015

Uh oh!

lsegal commented Mar 31, 2015

Uh oh!

sclasen Mar 31, 2015

Choose a reason for hiding this comment

Uh oh!

lsegal Mar 31, 2015

Choose a reason for hiding this comment

Uh oh!

sclasen commented Mar 31, 2015

Uh oh!

sclasen commented Apr 1, 2015

Uh oh!

danp commented Apr 1, 2015

Uh oh!

sclasen commented Apr 2, 2015

Uh oh!

lsegal commented Apr 6, 2015

Uh oh!

sclasen commented Apr 6, 2015

Uh oh!

sclasen commented Apr 11, 2015

Uh oh!

danp commented May 8, 2015

Uh oh!

lsegal commented May 12, 2015

Uh oh!

lsegal commented May 12, 2015

Uh oh!

Uh oh!