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

Explicit proto import search path #40

Merged
merged 2 commits into from May 20, 2018

Conversation

Projects
None yet
4 participants
@siddharthab
Contributor

siddharthab commented Jan 18, 2018

This tries to address #39.

The new function is more consistent with the behavior of protoc command line tool, and is easy enough for end users to get right. It expects that all provided paths are relative to an explicit search path.

With the previous function, the working dir was always added to the search path, and the files argument was always taken as an absolute path. I tried to change the existing function but it was difficult to be backward compatible in a clean way, hence the new function.

Open to suggestions on naming the new function.

@eddelbuettel

This comment has been minimized.

Owner

eddelbuettel commented Jan 18, 2018

That doesn't look unreasonable, and well localized. @murraystokely any views?

@murraystokely

This comment has been minimized.

Collaborator

murraystokely commented Jan 18, 2018

@eddelbuettel

This comment has been minimized.

Owner

eddelbuettel commented Jan 18, 2018

Unit tests are always good. In favor.

@siddharthab

This comment has been minimized.

Contributor

siddharthab commented Jan 18, 2018

Thank you. I just wanted some buy-in before I invested more into unit tests. I will send in an update soon.

@siddharthab

This comment has been minimized.

Contributor

siddharthab commented Jan 19, 2018

I tried to write unit tests but it is impossible to get a clean environment in the unit test setup. Every call to readProtoFiles adds directories to the search path that are never removed.

// source_tree.removeDirectories( dirs ) ;

As one use case of where the existing function gets tricky, I added a new proto file to a subdirectory in the unit tests in siddharthab@d780a38 and wrote unit tests using the new function. While there could be ways to make it work with the old function (mostly by changing working dir), I suspect they are not clean and intuitive. As of now, even the existing unit tests are failing because of the addition of this new import.

A rare use case that is impossible to make work with the existing function is when two directories have cyclical dependencies but are independent components in the search path. See siddharthab@0f5e650 for an example. This case can be easily handled with protoc and the new function.

Another advantage of the new function is that you will no longer require to retain the search path, and will be able to clear it after every call. Retaining the search path is also bug-prone when multiple files have same basename because the user will not realize that the importer is using a file from a previously added directory and not the file that he intends.

If you want, I can write up some more detailed test scripts so that we get a clean environment at the start of each script. Short of that, we will have to introduce utility methods to clear state, etc.

@siddharthab

This comment has been minimized.

Contributor

siddharthab commented Jan 19, 2018

As another example of how things are non-intuitive with existing function, this statement does not work when a user might expect it to work out of the box.

RProtoBuf::readProtoFiles(dir=system.file("unitTests", "data", package="RProtoBuf"))
@siddharthab

This comment has been minimized.

Contributor

siddharthab commented Apr 19, 2018

Ping.

Would you like me to simplify the counter examples I have provided?

@rangi513

This comment has been minimized.

rangi513 commented May 11, 2018

I found this PR very useful for a project I'm working on! Is this going to be merged in to the CRAN version sometime soon? In the meantime I'm using siddharthab's branch.

@eddelbuettel

This comment has been minimized.

Owner

eddelbuettel commented May 11, 2018

@murraystokely Any views? On the margin. it probably won't hurt. Just a new entry point...

@eddelbuettel

This comment has been minimized.

Owner

eddelbuettel commented May 20, 2018

I would have liked to hear @murraystokely on this, but looks like I won't.

As the patch cleanly extends the API and makes no changes to existing code, I see no harm. You should examples of existing code not allowing you to do what you wanted, so on balance this is a net benefit.

Thanks for the PR. I'll merge it now,

@eddelbuettel eddelbuettel merged commit 7c6da44 into eddelbuettel:master May 20, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@siddharthab

This comment has been minimized.

Contributor

siddharthab commented May 20, 2018

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment