Skip to content

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

Merged
TristonianJones merged 2 commits into
cel-expr:masterfrom
Alfus:alfus/fix_pkg
Aug 3, 2023
Merged

Update the proto, go, and c++ packages#302
TristonianJones merged 2 commits into
cel-expr:masterfrom
Alfus:alfus/fix_pkg

Conversation

@Alfus

@Alfus Alfus commented Jul 17, 2023

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Collaborator

/gcbrun

@jcking

jcking commented Jul 19, 2023

Copy link
Copy Markdown
Collaborator

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

Alfus commented Jul 20, 2023

Copy link
Copy Markdown
Contributor Author

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

Alfus commented Jul 20, 2023

Copy link
Copy Markdown
Contributor Author

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

@Alfus

Alfus commented Jul 20, 2023

Copy link
Copy Markdown
Contributor Author

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


syntax = "proto3";

package cel.expr;

Copy link
Copy Markdown
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?

@Alfus Alfus Jul 20, 2023

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Collaborator

/gcbrun

1 similar comment
@TristonianJones

Copy link
Copy Markdown
Collaborator

/gcbrun

@TristonianJones

Copy link
Copy Markdown
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

Alfus commented Aug 3, 2023

Copy link
Copy Markdown
Contributor Author

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

@TristonianJones

Copy link
Copy Markdown
Collaborator

/gcbrun

@TristonianJones
TristonianJones merged commit ed066e3 into cel-expr:master Aug 3, 2023
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.

4 participants