Kkaye/include paths #252
Kkaye/include paths #252
Conversation
|
Automated message from Dropbox CLA bot @GreatCall-KayeK, thanks for the pull request! It looks like you haven't yet signed the Dropbox CLA. Please sign it here. |
| @@ -69,7 +69,9 @@ fi | |||
| --objcpp-out "$temp_out/objc" \ | |||
| --objc-type-prefix TXS \ | |||
| \ | |||
| --idl "$in" | |||
| --idl "$in" \ | |||
| --idl-include-path "../../" \ | |||
There was a problem hiding this comment.
Are these necessary to build the example? If not, I'd rather leave them out.
|
Thanks for the PR @GreatCall-KayeK. I've added some suggestions in line notes. You'll also need to sign the CLA before I can merge your code. |
GreatCall-KayeK
force-pushed
the
kkaye/include-paths
branch
from
1f59930 to
3af6aaf
Compare
|
@artwyman My apologies for the long delay on this PR, the CLA got stuck in legal. I've signed that as well as implemented your feedback. Please let me know if you would like me rebase prior to merging this feature. |
|
No worries, you're not the one slowing things down. I'm way behind on PRs, but hope to have some time to catch up before the holidays. Rebase is optional, so long as you've resolved all conflicts. There aren't any major changes in master that I recall, which would make this harder to review. |
There was a problem hiding this comment.
Looks good overall. I made some suggestions in inline comments, and there area also some conflicts to resolve before I can merge this.
Note that I can confirm the CLA was signed. The company CLA (vs. individual) doesn't trigger the auto-response from the smarx-bot.
| def filePath = "[^\"]*".r | ||
|
|
||
| def fileParent(): String = { | ||
| if (fileStack.top.getParent() != null) fileStack.top.getParent() + "/" else "" |
There was a problem hiding this comment.
I think it would be better to pass paths around using the File type and use the joining functionality provided by the multi-arg File constructor rather than manipulating separators manually. It'll be more robust to Windows path separators, among other things.
There was a problem hiding this comment.
I've opted to remove this method. Additionally the path construction is now handled by the File constructor both here and in the importFile method.
| exists | ||
| }) | ||
|
|
||
| if (path.isEmpty || file.isEmpty) throw new FileNotFoundException("Unable to find file \"" + fileName + "\" at " + fileStack.top.getCanonicalPath) |
There was a problem hiding this comment.
Is the OR here necessary? Seems like the path variable is actually unnecessary, since the file variable gets the true result of the find()
| @import "./date.djinni" | ||
| @import "../djinni/date.djinni" | ||
| @extern "./date.yaml" | ||
| @import "./vendor/third-party/date.djinni" |
There was a problem hiding this comment.
This will need to be tweaked to merge conflicts. Note that some changes were made here to do relative-path testing using a different input file (not date/duration), since the Python code doesn't support external types yet, but wants to be able to test using all.djinni.
|
@artwyman Thank you for your feedback. I've implemented changes and resolved merge conflicts. |
This is an initial implementation of issue #244. Included is:
--idl-include-pathargument which specified a path to resolve@importand@externdirectives.--idl-include-pathin the order specified, until the import is found.FileNotFoundExceptionis thrown and the user is provided an error message.I look forward to hearing thoughts and feedback.