Don't use ObjectiveC-based handling of reserved keywords for Java #187

RicoYao · 2019-03-18T23:17:41Z

Java has its own set of different reserved keywords.
Also use a "uid" property name for "id" (ObjC uses "identifier). This is pinterest-specific and we may have to add in a custom override for this instead later.

maicki · 2019-03-19T01:10:07Z

Sources/Core/StringExtensions.swift

 extension String {
    func indent() -> String {
        // We indent with tabs and in a post process the tabs are changed to a specific number of spaces
        return "\t" + self
    }

    /// All components separated by _ will be capitalized including the first one
-    func snakeCaseToCamelCase() -> String {
+    func snakeCaseToCamelCase(_ language: Languages = Languages.objectiveC) -> String {


Some idea: What if we would create a "Language" Enum / Struct that knows that language Self is, e.g. either Objective-C, Java or some other language and that would contain the logic around reserved word replacement and capabilities such as creating snakeCaseToCamelCase, snakeCaseToPropertyName, snakeCaseToPropertyName. We could have one instance of language and passing this around and so we don't have to have this if else branching in a String extension as well as you don't have to pass in .java, .objectiveC everywhere as parameter. You would just call snakeCaseToCamelCase on the Language object you have in the Renderer.

Thanks. Moved to Languages enum.

rahul-malik · 2019-03-19T00:43:53Z

Sources/Core/StringExtensions.swift

@@ -137,35 +137,119 @@ let objectiveCReservedWords = Set<String>([
    "while",
 ])

+// TODO: "id" is technically allowed. It's possible not everyone wants this replacement.
+let javaReservedWordReplacements = [
+    "id": "uid",


Any particular reason for this? The mapping for ObjC is largely because “id” is a reserved word

As mentioned in a separate comment this is pinterest-specific.

rahul-malik · 2019-03-19T00:56:53Z

Sources/Core/StringExtensions.swift

        var str: String = self

-        if let replacementString = objectiveCReservedWordReplacements[self.lowercased()] as String? {
-            str = replacementString
+        if language == .objectiveC {


Please use a switch over if/else and if the language type is .flow we should do nothing

The benefit is exhaustive switching will raise errors as new languages are added

rahul-malik · 2019-03-19T01:42:56Z

Sources/Core/StringExtensions.swift

+
+// https://en.wikipedia.org/wiki/List_of_Java_keywords
+let javaReservedWords = Set<String>([
+    "abstract",


we should move these new functions as extensions on the language enum.

rahul-malik · 2019-03-19T19:29:11Z

@RicoYao

Also use a "uid" property name for "id" (ObjC uses "identifier). This is pinterest-specific and we may have to add in a custom override for this instead later.

Why is this necessary? Just lots of code that would need to be updated if this was changed?

RicoYao · 2019-03-19T21:01:19Z

@RicoYao

Also use a "uid" property name for "id" (ObjC uses "identifier). This is pinterest-specific and we may have to add in a custom override for this instead later.

Why is this necessary? Just lots of code that would need to be updated if this was changed?

In addition to code changes, there's a lot of institutional behavior around uid that would be painful to change.

- Java has its own set of different reserved keywords. - Also use a "uid" property name for "id" (ObjC uses "identifier). This is pinterest-specific and we may have to add in a custom override for this instead later.

…Languages enum

… stragglers. make format/lint.

rahul-malik · 2019-03-20T00:22:33Z

Sources/Core/FileGenerator.swift

+    "while",
+])
+
+// TODO: "id" is technically allowed. It's possible not everyone wants this replacement.


Can you file an issue to revisit this? I don't want to necessarily block internal progress but I think this warrants a further discussion

rahul-malik · 2019-03-20T00:24:42Z

Sources/Core/JavaFileRenderer.swift

@@ -90,7 +90,7 @@ extension JavaFileRenderer {
                fatalError("Bad reference found in schema for class: \(className)")
            }
        case .oneOf(types: _):
-            return "\(className)\(param.snakeCaseToCamelCase())"
+            return "\(className)\(Languages.java.snakeCaseToCamelCase(param))"


Can you file a ticket for abstracting the language type here ? I think the context of java / objc render should be able to influence which snakeCaseToCamelCase logic is used

Tests/CoreTests/LanguagesTests.swift

RicoYao · 2019-03-20T00:43:03Z

@rahul-malik Do you happen to know what I else I need to change to get the LinuxTestIndex to pass?

rahul-malik · 2019-03-20T00:45:54Z

@RicoYao - Did you run make build_test_linux_index? Linux TestRunner for Swift required generating classes that declare the test classes so we have a script to do this generation.

Please file a ticket to improve this since it's not obvious, I'll take care of making this a part of the lint step so it's a bit more obvious

rahul-malik · 2019-03-20T00:48:12Z

@RicoYao - I just looked at the failure log, run that make command and commit the changes and this should be fixed. sorry for the confusion!

RicoYao · 2019-03-20T01:14:05Z

Thanks @rahul-malik that did the trick! FYI, I also had to run make format to fix some issues on the generated changes from make build_test_index_linux.

RicoYao force-pushed the identifier_vs_uid branch from 9b03435 to c9c5b4c Compare March 18, 2019 23:22

maicki reviewed Mar 19, 2019

View reviewed changes

rahul-malik reviewed Mar 19, 2019

View reviewed changes

RicoYao added 2 commits March 19, 2019 14:22

Don't use ObjectiveC-based handling of reserved keywords for Java

c23556e

- Java has its own set of different reserved keywords. - Also use a "uid" property name for "id" (ObjC uses "identifier). This is pinterest-specific and we may have to add in a custom override for this instead later.

Move snakeCaseToPropertyName/CapitalizaedPropertyName/CamelCase into …

1b81b68

…Languages enum

RicoYao force-pushed the identifier_vs_uid branch from c9c5b4c to 1b81b68 Compare March 19, 2019 22:08

RicoYao added 5 commits March 19, 2019 15:12

Remove StringExtension methods that are now in Languages. Fixed a few…

d4e04a8

… stragglers. make format/lint.

Fix tests.

c3646ff

make format

71fd8c1

fix LinuxTestIndex

6ed6ccd

make format

7d9d24f

rahul-malik reviewed Mar 20, 2019

View reviewed changes

Tests/CoreTests/LanguagesTests.swift Show resolved Hide resolved

rahul-malik approved these changes Mar 20, 2019

View reviewed changes

This was referenced Mar 20, 2019

Java models are renaming "id" to "uid" #189

Open

We should not need to explicitly specify language for methods such as snakeCaseToPropertyName #190

Open

Add tests for renaming of "id" fields in java and objC

9980d50

make format

e51fefe

RicoYao added 2 commits March 19, 2019 17:50

Add linux test indexes (make build_test_index_linux)

13cd190

make format to fix the linux test indexes

f8f7af3

rahul-malik merged commit 5e5ed22 into pinterest:master Mar 20, 2019

This was referenced Mar 20, 2019

Handle conflicts like private within Java generated models #142

Closed

Create a uniform way for languages to express reserved keywords #84

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't use ObjectiveC-based handling of reserved keywords for Java #187

Don't use ObjectiveC-based handling of reserved keywords for Java #187

RicoYao commented Mar 18, 2019

maicki Mar 19, 2019

RicoYao Mar 19, 2019

rahul-malik Mar 19, 2019

RicoYao Mar 19, 2019

rahul-malik Mar 19, 2019

RicoYao Mar 19, 2019

rahul-malik Mar 19, 2019

RicoYao Mar 19, 2019

rahul-malik commented Mar 19, 2019

RicoYao commented Mar 19, 2019

rahul-malik Mar 20, 2019

RicoYao Mar 20, 2019

rahul-malik Mar 20, 2019

RicoYao Mar 20, 2019

RicoYao commented Mar 20, 2019

rahul-malik commented Mar 20, 2019 •

edited

Loading

rahul-malik commented Mar 20, 2019 •

edited

Loading

RicoYao commented Mar 20, 2019

Don't use ObjectiveC-based handling of reserved keywords for Java #187

Don't use ObjectiveC-based handling of reserved keywords for Java #187

Conversation

RicoYao commented Mar 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahul-malik commented Mar 19, 2019

RicoYao commented Mar 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RicoYao commented Mar 20, 2019

rahul-malik commented Mar 20, 2019 • edited Loading

rahul-malik commented Mar 20, 2019 • edited Loading

RicoYao commented Mar 20, 2019

rahul-malik commented Mar 20, 2019 •

edited

Loading

rahul-malik commented Mar 20, 2019 •

edited

Loading