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

Expand list of reserved objc keywords to cover most conflicting cases… #79

Merged
merged 15 commits into from
Sep 24, 2017
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions .buildkite/plank-pipeline.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
steps:
- name: ":docker: Build base image with sources"
plugins:
docker-compose#v1.5.0:
build: app
image-repository: buildkite-registry.pinadmin.com/registry
volumes:
- ".:/app/mnt/buildkite-builds/volumes"
config: docker-compose.yml
agents:
queue: pinboard
- wait: ~
- name: "Build :iphone: :airplane:"
command: "swift build"
timeout_in_minutes: 30
agents:
queue: pinboard
plugins:
docker-compose#v1.5.0:
run: app
config: docker-compose.yml
- name: "Test :iphone: :airplane:"
command: "swift test"
timeout_in_minutes: 30
agents:
queue: pinboard
plugins:
docker-compose#v1.5.0:
run: app
config: docker-compose.yml
- name: "Integration Test :iphone: :airplane:"
command: "swift build && ./Utility/integration-test.sh"

Choose a reason for hiding this comment

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

You probably want to move swift build either into the Docker container (if you're adding all files), or run this before integration-test.sh.

You might also create a single shell script that wraps both of these commands, but I think you can only run a single thing in the command here.

Choose a reason for hiding this comment

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

I just saw that you were copying everything into the docker container with COPY . /plank. Because we're copying files this will invalidate the docker cache for basically every PR, which is fine, but might mean that we want to also run swift build inside of the docker container. This would also let you use the build for other steps in the pipeline if needed in parallel to integration tests.

timeout_in_minutes: 30
agents:
queue: pinboard
plugins:
docker-compose#v1.5.0:
run: app
config: docker-compose.yml
2 changes: 1 addition & 1 deletion .swift-version
Original file line number Diff line number Diff line change
@@ -1 +1 @@
3.1
4.0
4 changes: 2 additions & 2 deletions BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@ swift_library(
name = "PlankCore",
srcs = glob(["Sources/Core/*.swift"]),
module_name = "Core",
swift_version = 3,
swift_version = 4,
copts = ["-whole-module-optimization"]
)

swift_library(
name = "PlankLib",
srcs = glob(["Sources/plank/*.swift"]),
swift_version = 3,
swift_version = 4,
copts = ["-whole-module-optimization"],
deps = [":PlankCore"]
)
Expand Down
18 changes: 7 additions & 11 deletions Dockerfile
Original file line number Diff line number Diff line change
@@ -1,21 +1,17 @@
# Base image from Swiftenv with Swift version 3.0.2

FROM kylef/swiftenv:latest
# Base image from SwiftDocker
# https://hub.docker.com/r/swiftdocker/swift/
FROM swiftdocker/swift
MAINTAINER Pinterest
RUN swiftenv install 3.1

# Vim config so we have an editor available
RUN apt-get update && \
apt-get install -y --no-install-recommends \
vim clang libicu-dev libcurl4-openssl-dev libssl-dev

# Install plank
COPY . /usr/local/plank
RUN cd /usr/local/plank && swift build -c release

ENV plank_HOME /usr/local/plank
ENV PATH ${plank_HOME}/.build/release:${PATH}

# Uncomment to make `plank` the default action of `docker run [image_name]`
#ENTRYPOINT ["plank"]
#CMD ["help"]
# Copy plank sources
COPY . /plank
WORKDIR /plank

7 changes: 4 additions & 3 deletions Examples/Cocoa/Tests/ObjcTests/ObjcTests.swift
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
import Foundation
import XCTest

@testable import objc
@testable import Objc

// Helper for comparing model dictionaries
public func ==(lhs: [AnyHashable: Any], rhs: [AnyHashable: Any] ) -> Bool {
return NSDictionary(dictionary: lhs).isEqual(to: rhs)
}

class ObjcTests: XCTestCase {
class ObjcTestSuite: XCTestCase {

func testBasicObjectInitialization() {
let imageModelDictionary: [AnyHashable: Any] = [
"height": 12,
Expand Down
5 changes: 4 additions & 1 deletion Sources/Core/JSIR.swift
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,10 @@ public struct JSIR {
return [
JSRuntimeFile.runtimeImports(),
// TODO: JS: We should find a better way to remove a cyclic import as filtering it here
classNames.filter { $0 != "\(myName)Type" }.sorted().map { JSIR.fileImportStmt($0, $0) }.joined(separator: "\n")
(classNames.filter { $0 != "\(myName)Type" } as [String])
.sorted()
.map { JSIR.fileImportStmt($0, $0) }
.joined(separator: "\n")
]
case .typeDecl(name: let className, extends: _, properties: let properties):
let nullability = { (prop: SchemaObjectProperty) -> String in
Expand Down
4 changes: 3 additions & 1 deletion Sources/Core/ObjectiveCEqualityExtension.swift
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,9 @@ extension ObjCFileRenderer {
}

// Performance optimization - compare primitives before resorting to more expensive `isEqual` calls
let sortedProps = self.properties.sorted { $0.0.1.schema.isObjCPrimitiveType }
let sortedProps = self.properties.sorted { (arg1, _) in
arg1.1.schema.isObjCPrimitiveType
}

let propReturnStmts = sortedProps.map { param, prop -> String in
let formattedParam = param.snakeCaseToPropertyName()
Expand Down
3 changes: 2 additions & 1 deletion Sources/Core/ObjectiveCInitExtension.swift
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,8 @@ extension ObjCModelRenderer {
if let assignmentLine = lines.last {
let propAssignmentPrefix = "\(propertyToAssign) = "
if assignmentLine.hasPrefix(propAssignmentPrefix) {
let propertyInitStatement = assignmentLine.substring(from: propAssignmentPrefix.endIndex).trimmingCharacters(in: CharacterSet.init(charactersIn: " ;"))
let startIndex = propAssignmentPrefix.endIndex
let propertyInitStatement = String(assignmentLine[startIndex...]).trimmingCharacters(in: CharacterSet(charactersIn: " ;"))
let adtInitStatement = propAssignmentPrefix + "[\(adtClassName) objectWith\(ObjCADTRenderer.objectName(schema)):\(propertyInitStatement)];"
return lines.dropLast() + [adtInitStatement]
}
Expand Down
98 changes: 93 additions & 5 deletions Sources/Core/StringExtensions.swift
Original file line number Diff line number Diff line change
Expand Up @@ -52,31 +52,114 @@ prefix func --> (body: () -> [String]) -> String {
return -->body()
}

// Most of these are derived from https://www.binpress.com/tutorial/objective-c-reserved-keywords/43
Copy link
Contributor

Choose a reason for hiding this comment

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

We should create some abstraction for each language. Could be for example a Language protocol that will be passes around and every supported language creates a struct that conforms to it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@maicki - I agree since there will be different reserved words per language. Only caveat is that the names must be consistent for any languages that bridge (Objc, Flow, Swift, etc.) so we might need a global set of all languages at runtime to utilize for consistent results.

// other language conflicts should be ideally added here.
// TODO: Find a way to separate this by language since the reserved keywords will differ.
let objectiveCReservedWordReplacements = [
"description": "description_text",
"id": "identifier"
// TODO: Fill out more objc keywords with replacements.
]

let objectiveCReservedWords = Set<String>([
"@catch()",
"@class",
"@dynamic",
"@end",
"@finally",
"@implementation",
"@interface",
"@private",
"@property",
"@protected",
"@protocol",
"@public",
"@selector",
"@synthesize",
"@throw",
"@try",
"BOOL",
"Class",
"IMP",
"NO",
"NULL",
"Protocol",
"SEL",
"YES",
"_Bool",
"_Complex",
"_Imaginery",
"atomic",
"auto",
"break",
"bycopy",
"byref",
"case",
"char",
"const",
"continue",
"default",
"do",
"double",
"else",
"enum",
"extern",
"float",
"for",
"goto",
"id",
"if",
"in",
"inline",
"inout",
"int",
"long",
"nil",
"nonatomic",
"oneway",
"out",
"register",
"restrict",
"retain",
"return",
"self",
"short",
"signed",
"sizeof",
"static",
"struct",
"super",
"switch",
"typedef",
"union",
"unsigned",
"void",
"volatile",
"while"
])

extension String {
/// All components separated by _ will be capitalized including the first one
func snakeCaseToCamelCase() -> String {
var str: String = self

if let replacementString = objectiveCReservedWordReplacements[self] as String? {
if let replacementString = objectiveCReservedWordReplacements[self.lowercased()] as String? {
str = replacementString
}

let components = str.components(separatedBy: "_")
let name = components.map { return $0.uppercaseFirst }
return name.joined(separator: "")
let formattedName = name.joined(separator: "")
if objectiveCReservedWords.contains(formattedName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this clash if you would like to use "void" for something but it's a reserved word in objective c?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if you used "void" it would just become "voidProperty" or "VoidProperty"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The purpose of this PR is to prevent reserved words from being generated as identifiers. I don't believe this will cause issues for most since naming anything as a reserved keyword is probably unlikely.

return "\(formattedName)Property"
}
return formattedName
}

/// All components separated by _ will be capitalized execpt the first
func snakeCaseToPropertyName() -> String {
var str: String = self

if let replacementString = objectiveCReservedWordReplacements[self] as String? {
if let replacementString = objectiveCReservedWordReplacements[self.lowercased()] as String? {
str = replacementString
}

Expand All @@ -97,6 +180,10 @@ extension String {
}
}

if objectiveCReservedWords.contains(name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question as above?

return "\(name)Property"
}

return name
}

Expand All @@ -108,7 +195,8 @@ extension String {

/// Get the last n characters of a string
func suffixSubstring(_ length: Int) -> String {
return self.substring(from: self.characters.index(self.endIndex, offsetBy: -length))
let index = self.characters.index(self.endIndex, offsetBy: -length)
return String(self[index...])
}

/// Uppercase the first character
Expand Down
5 changes: 2 additions & 3 deletions Tests/CoreTests/LinuxTestIndex.swift
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@ extension ObjectiveCIRTests {
static var allTests = [
("testDirtyPropertyOption", testDirtyPropertyOption),
("testDirtyPropertyOptionMultiWord", testDirtyPropertyOptionMultiWord),
("testDirtyPropertyOptionThrowsForEmptyProp", testDirtyPropertyOptionThrowsForEmptyProp),
("testDirtyPropertyOptionThrowsForEmptyClass", testDirtyPropertyOptionThrowsForEmptyClass),
("testEnumTypeName", testEnumTypeName),
("testEnumToStringName", testEnumToStringName),
("testEnumFromStringName", testEnumFromStringName),
Expand All @@ -37,7 +35,8 @@ extension StringExtensionsTests {
("testSuffixSubstring", testSuffixSubstring),
("testSnakeCaseToCamelCase", testSnakeCaseToCamelCase),
("testSnakeCaseToPropertyName", testSnakeCaseToPropertyName),
("testSnakeCaseToCapitalizedPropertyName", testSnakeCaseToCapitalizedPropertyName)
("testSnakeCaseToCapitalizedPropertyName", testSnakeCaseToCapitalizedPropertyName),
("testReservedKeywordSubstitution", testReservedKeywordSubstitution)
]
}
#endif
9 changes: 0 additions & 9 deletions Tests/CoreTests/ObjectiveCIRTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,6 @@ class ObjectiveCIRTests: XCTestCase {
let optionName = dirtyPropertyOption(propertyName: "some_prop", className: "class")
XCTAssertEqual(optionName, "classDirtyPropertySomeProp")
}
// Figure out how to test fatalErrors or convert dirtyPropertyOption() to return a Result<String, Err> type
// func testDirtyPropertyOptionThrowsForEmptyProp() {
// XCTAssertThrowsError(dirtyPropertyOption(propertyName: "", className: "class"))
// }
//
// func testDirtyPropertyOptionThrowsForEmptyClass() {
//
// XCTAssertThrowsError(dirtyPropertyOption(propertyName: "", className: "class"))
// }

func testEnumTypeName() {
XCTAssertEqual(enumTypeName(propertyName: "some_prop", className: "class"), "classSomePropType")
Expand Down
4 changes: 4 additions & 0 deletions Tests/CoreTests/StringExtensionsTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -49,4 +49,8 @@ class StringExtensionsTests: XCTestCase {
XCTAssert("created_at".snakeCaseToCapitalizedPropertyName() == "CreatedAt")
XCTAssert("CreatedAt".snakeCaseToCapitalizedPropertyName() == "CreatedAt")
}

func testReservedKeywordSubstitution() {
XCTAssert("nil".snakeCaseToCapitalizedPropertyName() == "NilProperty")
}
}
10 changes: 6 additions & 4 deletions Utility/integration-test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,20 @@ JSON_FILES=`ls -d Examples/PDK/*.json`
# Generate flow types for models
.build/debug/plank --lang flow --output_dir=Examples/JS/flow/ $JSON_FILES

ROOT_DIR="${PWD}"

# Verify flow types
if [ -x "$(command -v flow)" ]; then
pushd Examples/JS/flow
cd Examples/JS/flow
flow
popd
cd "${ROOT_DIR}"
fi

# Move headers in the right place for the Swift PM
mv Examples/Cocoa/Sources/objc/*.h Examples/Cocoa/Sources/objc/include

# Build the ObjC library
pushd Examples/Cocoa
cd Examples/Cocoa
swift build
swift test
popd
cd "${ROOT_DIR}"
21 changes: 21 additions & 0 deletions docker-compose.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
version: '2.1'
services:
app:
shm_size: 2g
build: .
tmpfs: /tmp
volumes:
- ./:/plank
environment:
BUILDKITE_BRANCH:
BUILDKITE_BUILD_URL:
BUILDKITE_COMMIT:
BUILDKITE_MESSAGE:
BUILDKITE_PIPELINE_SLUG:
BUILDKITE_AGENT_ACCESS_TOKEN:
BUILDKITE_JOB_ID:
BUILDKITE_BUILD_ID:
BUILDKITE_BUILD_NUMBER:
BUILDKITE_PARALLEL_JOB_COUNT:
BUILDKITE_PARALLEL_JOB:
network_mode: "host"
Loading