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

Allow edition parsing #4

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lizard-boy
Copy link

defaulting to proto2 behavior, which is kinda wrong.

The way we break the world for Java callers is very sad...
Maybe we could just manually increate the enum values one bye one each year...

oldergod added 2 commits June 21, 2024 13:15
defaulting to proto2 behavior
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR introduces support for Protocol Buffers editions in the Wire library, focusing on maintaining backward compatibility while allowing for future editions. Key changes include:

  • Modified Syntax class from enum to sealed class in wire-runtime/src/commonMain/kotlin/com/squareup/wire/Syntax.kt
  • Added support for parsing 'edition' declarations in wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/internal/parser/ProtoParser.kt
  • Updated ProtoFileElement to handle 'edition' keyword in wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/internal/parser/ProtoFileElement.kt
  • Modified SyntaxRules to accommodate new Syntax.Edition case in wire-schema/src/commonMain/kotlin/com/squareup/wire/schema/SyntaxRules.kt
  • Updated various Java files to use Syntax.PROTO_2.INSTANCE instead of Syntax.PROTO_2

30 file(s) reviewed, 14 comment(s)
Edit PR Review Bot Settings

Comment on lines +31 to +33
public static final Double DEFAULT_LENGTH_METERS = 0.0d;

public static final Double DEFAULT_MASS_KILOGRAMS = 0.0d;
Copy link

Choose a reason for hiding this comment

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

style: Consider using BigDecimal for precise representation of floating-point numbers

case 1: return CRETACEOUS;
case 2: return JURASSIC;
case 3: return TRIASSIC;
default: return null;
Copy link

Choose a reason for hiding this comment

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

style: Returning null for unknown values might lead to NullPointerExceptions. Consider throwing an IllegalArgumentException instead.

@@ -0,0 +1,17 @@
syntax = "proto2";

package squareup.java.dinosaurs;
Copy link

Choose a reason for hiding this comment

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

style: Package name differs from other similar files. Consider using 'squareup.dinosaurs' for consistency.


package squareup.java.dinosaurs;

import "squareup/wire/java/geology/period.proto";
Copy link

Choose a reason for hiding this comment

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

style: Import path differs from other files. Consider using 'squareup/geology/period.proto' for consistency.


optional double length_meters = 3;
optional double mass_kilograms = 4;
optional squareup.java.geology.Period period = 5;
Copy link

Choose a reason for hiding this comment

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

logic: Period type should be 'squareup.geology.Period' to match the typical package structure.

Comment on lines +42 to 47
@Deprecated("Use get(string: String, edition: Boolean) instead", ReplaceWith("Syntax.get(string, edition)"))
operator fun get(string: String): Syntax {
for (syntax in values()) {
if (syntax.string == string) return syntax
}
if (string == PROTO_2.toString()) return PROTO_2
if (string == PROTO_3.toString()) return PROTO_3
throw IllegalArgumentException("unexpected syntax: $string")
}
Copy link

Choose a reason for hiding this comment

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

logic: The deprecated get() function doesn't handle Edition cases, which might cause issues for existing code

Comment on lines +49 to +58
fun get(string: String, edition: Boolean): Syntax {
if (edition) {
if (string in KNOWN_EDITIONS) return Edition(string)
throw IllegalArgumentException("unknown edition: $string")
} else {
if (string == PROTO_2.toString()) return PROTO_2
if (string == PROTO_3.toString()) return PROTO_3
throw IllegalArgumentException("unexpected syntax: $string")
}
}
Copy link

Choose a reason for hiding this comment

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

style: Consider using a when expression for better readability and maintainability

Comment on lines +41 to +44
when (syntax) {
is Edition -> append("edition")
else -> append("syntax")
}
Copy link

Choose a reason for hiding this comment

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

logic: This when expression doesn't handle all possible cases of Syntax. Consider adding an else branch to handle unexpected types

Comment on lines +124 to +125
(label == "syntax" || label == "edition") && context.permitsSyntax() -> {
reader.expect(syntax == null, location) { "too many syntax or edition definitions" }
Copy link

Choose a reason for hiding this comment

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

style: Consider separating the handling of 'syntax' and 'edition' into separate conditions for clarity

label == "syntax" && context.permitsSyntax() -> {
reader.expect(syntax == null, location) { "too many syntax definitions" }
(label == "syntax" || label == "edition") && context.permitsSyntax() -> {
reader.expect(syntax == null, location) { "too many syntax or edition definitions" }
Copy link

Choose a reason for hiding this comment

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

style: Update error message to mention both 'syntax' and 'edition'

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.

2 participants