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.

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

Add new file option php_namespace. #3162

Merged
merged 5 commits into from
Jun 5, 2017
Merged

Conversation

TeBoring
Copy link
Contributor

Use this option to change the namespace of php generated classes.
Default is empty. When this option is empty, the package name will be
used for determining the namespace.

Use this option to change the namespace of php generated classes.
Default is empty. When this option is empty, the package name will be
used for determining the namespace.
@bazel-io
Copy link

Can one of the admins verify this patch?

@TeBoring
Copy link
Contributor Author

#3122

@TeBoring
Copy link
Contributor Author

@michaelbausor

Copy link
Contributor

@michaelbausor michaelbausor left a comment

Choose a reason for hiding this comment

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

Thanks a lot, this is great! I did not try to review as I am not familiar with the codebase, but did notice some commented tests.

$end = memory_get_usage();
$this->assertLessThan($start, $end);
}
# /**
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these tests supposed to be commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@TeBoring
Copy link
Contributor Author

Thanks for the catch.

@michaelbausor
Copy link
Contributor

Testing locally, I am finding that this does not change the namespace of the GPBMetadata files. Is that intentional? I do not know of strong arguments either for changing it or not.

@TeBoring
Copy link
Contributor Author

TeBoring commented Jun 1, 2017

I don't think we need to change the namespace of GPBMetadata files. These files are transparent to users.

@TeBoring
Copy link
Contributor Author

TeBoring commented Jun 1, 2017

BTW, the namespace of GPBMetadata files are decided by the path of proto file instead of the package name.

@TeBoring TeBoring merged commit 6f32580 into protocolbuffers:master Jun 5, 2017
TeBoring added a commit to TeBoring/protobuf that referenced this pull request Jun 20, 2017
* Add new file option php_namespace.

Use this option to change the namespace of php generated classes.
Default is empty. When this option is empty, the package name will be
used for determining the namespace.

* Uncomment commented tests

* Revert gdb test change

* Update csharp descriptor.

* Add test for empty php_namespace.
@heyanlong
Copy link

@TeBoring What time php_namespace. protoc 3.4?

@TeBoring
Copy link
Contributor Author

ETA end of July.

bithium pushed a commit to bithium/protobuf that referenced this pull request Sep 4, 2023
* Add new file option php_namespace.

Use this option to change the namespace of php generated classes.
Default is empty. When this option is empty, the package name will be
used for determining the namespace.

* Uncomment commented tests

* Revert gdb test change

* Update csharp descriptor.

* Add test for empty php_namespace.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants