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

Clarify protobuf/Jackson examples in README #403

Merged
merged 2 commits into from
Jun 7, 2023

Conversation

snazy
Copy link
Member

@snazy snazy commented Jun 6, 2023

No description provided.

dependencies {
api(project(":cel-core"))

testImplementation(platform(libs.junit.bom))
testImplementation(libs.bundles.junit.testing)
testRuntimeOnly(libs.junit.jupiter.engine)
}

// *.proto files taken from https://github.com/googleapis/googleapis/ repo, available as a git
// submodule
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure i understand what the comment is trying to tell me, what proto files are we referring to here?

Copy link
Member Author

Choose a reason for hiding this comment

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

copy-paste-mess

README.md Outdated

```java
public class MyClass {
public Boolean evalWIthProtobuf() {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: typo With

README.md Outdated
.buildScript("inp.Property1 == checkName")
.withDeclarations(
// protobuf types need the type's full name
Decls.newVar("inp", Decls.newObjectType(Dummy.MyPojo.getDescriptor().getFullName())),
Copy link
Contributor

Choose a reason for hiding this comment

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

might be unclear where Dummy is coming from, doesnt it depend on the proto filename?
in our tests its clear in the example it isnt imo

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated the readme (this "Dummy" came from the .proto file name in the test)

@snazy snazy merged commit e78072b into projectnessie:main Jun 7, 2023
4 checks passed
@snazy snazy deleted the clarify-readme branch June 7, 2023 08:46
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

2 participants