Resolve self referencing types.

[pinsri]

Problem

Scrooge currently doesn't support self-referencing types. This is useful for a number of cases.

Solution

Change the type resolver's scope to include the current type. Add unit tests. Updated gold file and test.thrift to include a self-referencing type. Fixed a bug in GoldFileTest.scala's diff calculation code where if genStr is a prefix of expected, line 108 (getStr(i)) will generate OutOfBoundsException (since the diff is after the genStr string).

Result

Self-referencing types no longer result in TypeNotFoundException. This should resolve #243 and partially resolve #160. This change doesn't yet support recursive types which is needed to fully resolve #160.

This should be a backward compatible change since all the thrift files that are resolved correctly now will continue to get resolved correctly.

One caveat though is that we don't enforce the self-reference type to be optional or collection type. We just depend on the generated language's compiler to enforce that (i.e. we expect that compiler to generate an error if a self-referencing type is specified as required).

[kevinoliver]

@pinsri thanks! the change seems small enough. regardless, i'm nervous!

to alleviate my paranoia, can you add 2 more tests? first where you add a self-reference to one of the IDLs under scrooge-generator-tests/src/test/thrift? second, modify a gold file IDL in scrooge-generator-tests/src/test/resources/gold_file_input so we can see what this does to the generated code.

thanks again.

[pinsri]

let me try. thanks.

[codecov-io]

Current coverage is 27.27% (diff: 100%)

No coverage report found for develop at 178491d.

Powered by Codecov. Last update 178491d...47b8c1d

[mosesn]

LGTM

[pinsri]

Added 2 more tests. Adding to gold file input took way way more time than I expected. I eventually hacked up GoldFileTest.scala to print the full path of the generated file and not delete the temp directory and copied those files back (with some cleanup needed as well). Is there some simpler way to generate gold outputs that I'm missing?

[kevinoliver]

Thanks @pinsri. Yeah, the gold file test is a pain. When I've done it in the past, I thought it logged the new output to stdout (see https://github.com/twitter/scrooge/blob/develop/scrooge-generator-tests/src/test/scala/com/twitter/scrooge/goldfile/GoldFileTest.scala#L115-L128). Maybe that wasn't working?

[pinsri]

It was outputting to stdout, but copy pasting introduced unnecessary spaces
for me.

On Sep 21, 2016 10:31 AM, "Kevin Oliver" [email protected] wrote:

Thanks @pinsri https://github.com/pinsri. Yeah, the gold file test is a
pain. When I've done it in the past, I thought it logged the new output to
stdout (see https://github.com/twitter/scrooge/blob/develop/scrooge-
generator-tests/src/test/scala/com/twitter/scrooge/
goldfile/GoldFileTest.scala#L115-L128). Maybe that wasn't working?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#244 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/APGppYXlvc3ZoL-LEnUlXaQCAX-Q7nqyks5qsWoAgaJpZM4KCOKI
.

[pinsri]

@kevinoliver : does the PR look ok to you now?

[kevinoliver]

@pinsri yep! working on getting it merged in right now. thanks for the patch.

[pinsri]

Great, thanks for your review, @kevinoliver

[pinsri]

Also, do you know when the next release is going to be?

[kevinoliver]

We are targeting the beginning of October for the next release.

[kevinoliver]

Got this merged and it should show up on the develop branch this Monday. Thanks again.

[mosesn]

@pinsri this made it to develop here: 2271a73 thanks for the contribution!

[pinsri]

@mosesn @kevinoliver Is the release going to be this week? Thanks.

[kevinoliver]

@pinsri Yep, looking very likely. There is one OSS issue we are trying to get sorted out before hand.