Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add option to have require_relative statements #377

Merged
merged 1 commit into from
Sep 6, 2017
Merged

Add option to have require_relative statements #377

merged 1 commit into from
Sep 6, 2017

Conversation

tdeo
Copy link
Contributor

@tdeo tdeo commented Jul 23, 2017

To account for this issue: #240.

I chose to use an environment variable REQUIRE_RELATIVE=true as I can't see another way to pass options to the gem

@tdeo
Copy link
Contributor Author

tdeo commented Jul 24, 2017

@film42 let me know if that's a good solution and I'll add instructions in the readme for that.

film42
film42 requested changes Aug 7, 2017
View reviewed changes
lib/protobuf/generators/file_generator.rb Outdated
@@ -125,7 +125,7 @@ def print_import_requires
header "Imports"

descriptor.dependency.each do |dependency|
print_require(convert_filename(dependency))
print_require(convert_filename(dependency), ENV['REQUIRE_RELATIVE'] == 'true')
Copy link
Member

Choose a reason for hiding this comment

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

We've been prefacing env vars with PB_. Would you mind doing that here? PB_REQUIRE_RELATIVE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

@film42
Copy link
Member

film42 commented Aug 7, 2017

Sorry for the delay in getting this reviewed btw! Thanks for your contribution ❤️ !

@tdeo
Copy link
Contributor Author

tdeo commented Aug 7, 2017

I just changed the code to change the environment variable name to PB_REQUIRE_RELATIVE and to only be checked for presence as the others. I'll edit the wiki when it gets merged.

Thanks

film42
film42 approved these changes Aug 7, 2017
View reviewed changes
Copy link
Member

@film42 film42 left a comment

Choose a reason for hiding this comment

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

❤️ 🎉 ❤️ 🎉 ❤️ 🎉 ❤️ 🎉 ❤️ 🎉 ❤️

@tdeo
Copy link
Contributor Author

tdeo commented Aug 23, 2017

@film42 Do you have any idea when you'll be able to merge this / release ? Thanks

@film42
Copy link
Member

film42 commented Aug 23, 2017

@tdeo I'll release this right after we get map types released #367.

@ojab
Copy link

ojab commented Aug 30, 2017

I've almost sent a PR that adds option to use require_dependency instead (it helps with Rails autoload + protobuf imports), but I assume that it's better to tweak this PR and have just one option (something like PB_REQUIRE_KEYWORD that can be require_relative, require_dependency or whatever else).
Sounds good, can be done?

And, btw, print_require is used in print_generic_requires, I think that it'll break with require_relative and alike.

@ojab
Copy link

ojab commented Aug 30, 2017

Ah, please ignore statement about print_generic_requires, after rereading the patch I see that relative is passed only for imports.

@film42 film42 merged commit b9c7140 into ruby-protobuf:master Sep 6, 2017
@film42
Copy link
Member

film42 commented Sep 6, 2017

Released in v3.8.0! Thanks for the contribution!

@tdeo tdeo deleted the require_relative branch September 7, 2017 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@film42 film42 film42 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tdeo @film42 @ojab