Skip to content
This repository has been archived by the owner on Mar 8, 2023. It is now read-only.

Add support for including other Thrift files. #72

Merged
merged 4 commits into from
Nov 3, 2015
Merged

Add support for including other Thrift files. #72

merged 4 commits into from
Nov 3, 2015

Conversation

abhinav
Copy link
Contributor

@abhinav abhinav commented Oct 30, 2015

Renamed .{services,types,constants} on generated modules to have __ at start
and end to avoid naming conflicts.

Removed force flag from load(). The caching behavior is relied upon by the
include logic. Users can instantiate new Loaders if they need the result to
not be cached.

Update: After discussion, we decided that relative includes must always start with ./ or ../. This makes it clearer that the imports are relative to the .thrift file's directory and also allows us to later add different behavior around when ./ and ../ are not specified (like a thrift path environment variable).

@abhinav
Copy link
Contributor Author

abhinav commented Oct 30, 2015

@blampe @breerly @junchaoWu

@abhinav abhinav mentioned this pull request Oct 30, 2015
Renamed .{services,types,constants} on generated modules to have `__` at start
and end to avoid naming conflicts.

Removed force flag from load(). The caching behavior is relied upon by the
include logic. Users can instantiate new Loaders if they need the result to
not be cached.
@lucasmarshall
Copy link

Something I did in my implementation of this feature was to allow includes in IDLs loaded via loads(). The paths were interpreted as relative to the current working directory. Might be something potentially useful to add, but it shouldn't block landing this.

@abhinav
Copy link
Contributor Author

abhinav commented Oct 30, 2015

@lucasmarshall Created #73 to add that in the future since it doesn't seem like an immediate issue.

This clarifies that the includes are relative and ensures that the version
without the "./" or "../" is reserved for later if we decide to add a thrift
path or something similar.
@abhinav
Copy link
Contributor Author

abhinav commented Nov 2, 2015

Just an update: Based on discussion with @kriskowal and @malandrew, a slight behavioral change was added: Relative includes must start with ./ or ../. Since we only support relative includes right now, include "foo.thrift" will raise an exception.

@@ -4,6 +4,8 @@ Releases
0.6.0 (unreleased)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should move this to 1.0.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I'll let zest.releaser take care of that.

@blampe
Copy link
Contributor

blampe commented Nov 3, 2015

👍 amazing

abhinav added a commit that referenced this pull request Nov 3, 2015
Add support for including other Thrift files.
@abhinav abhinav merged commit e33a58d into master Nov 3, 2015
@abhinav abhinav deleted the imports branch November 3, 2015 17:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants