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

Update the proto, go, and c++ packages #302

Merged
merged 2 commits into from
Aug 3, 2023

Conversation

Alfus
Copy link
Contributor

@Alfus Alfus commented Jul 17, 2023

To match the following:

  • proto: cel.expr.* / "cel/expr/*"
  • c++: cel::expr::* / "cel/expr/*"
  • go: cel.dev/expr
  • java: dev.cel.expr (unchanged)

The tld is typically not included in proto/c++, and was in the wrong spot for the go package.

This leaves the existing protos inplace, to allow for a transition to the new packages.

@TristonianJones
Copy link
Collaborator

/gcbrun

@jcking
Copy link
Collaborator

jcking commented Jul 19, 2023

Not sure I am super excited about the C++ namespace cel::expr. It's a bit redundant, almost like saying ATM machine.

Maybe we are better off adding a suffix to all the top-level messages/enums, such as Proto? That way it is guaranteed to not collide with native names.

@Alfus
Copy link
Contributor Author

Alfus commented Jul 20, 2023

cel::expr is the 'expression' part of the C(E)L. There should also be a parser and an evaluator for the full language, for example.

@Alfus
Copy link
Contributor Author

Alfus commented Jul 20, 2023

A rename of the messages would be a much larger transition for the open source libraries that have already adopted these protos.

@Alfus
Copy link
Contributor Author

Alfus commented Jul 20, 2023

(also, pretty sure you need at least one sub package in go and maybe python)


syntax = "proto3";

package cel.expr;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we not including the tld in the main proto package name?

Copy link
Contributor Author

@Alfus Alfus Jul 20, 2023

Choose a reason for hiding this comment

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

The real requirement is: The proto package should match both the c++ namespace and the folders.

This is the (defacto?) standard way to reconcile that c++, python, es, etc, do not typically include the tld in namespaces/packages. It is only go (which uses it as real dns address) and java (which shoves the rdomain in the namespace per the style guide) that use the tld.

It matches the googleapis repo, for example see:
https://github.com/googleapis/googleapis/blob/master/google/api/log.proto#L24

@TristonianJones
Copy link
Collaborator

/gcbrun

1 similar comment
@TristonianJones
Copy link
Collaborator

/gcbrun

@TristonianJones
Copy link
Collaborator

@Alfus can you sync with HEAD and push again? I've updated the config for GCB, but I don't think it'll take in this PR until its synced.

@Alfus
Copy link
Contributor Author

Alfus commented Aug 3, 2023

I could also delete some of the old definitions. It seems to just be a lot to compute

@TristonianJones
Copy link
Collaborator

/gcbrun

@TristonianJones TristonianJones merged commit ed066e3 into google:master Aug 3, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants