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

Conversation

rahul-malik
Copy link
Collaborator

…. This is

an incomplete list and will likely have more additions in the future.

@rahul-malik
Copy link
Collaborator Author

@bkase - this breaks the integration tests so I'll need to take another pass at it

@CavalcanteLeo
Copy link

Could you guys accept this PR, I really need this to work.

Thank you very much

@ghost
Copy link

ghost commented Sep 18, 2017

🚫 CI failed with log

@rahul-malik
Copy link
Collaborator Author

@CavalcanteLeo - Looking into this currently. We had an issue with the first version that needed to be resolved to merge.

@ghost
Copy link

ghost commented Sep 18, 2017

🚫 CI failed with log

@ghost
Copy link

ghost commented Sep 19, 2017

🚫 CI failed with log

@ghost
Copy link

ghost commented Sep 19, 2017

🚫 CI failed with log

@CavalcanteLeo
Copy link

Ok, thanks dude!

@ghost
Copy link

ghost commented Sep 20, 2017

🚫 CI failed with log

@ghost
Copy link

ghost commented Sep 20, 2017

🚫 CI failed with log

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.

@ghost
Copy link

ghost commented Sep 24, 2017

🚫 CI failed with log

@@ -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.

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.

@@ -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?

@ghost
Copy link

ghost commented Sep 24, 2017

🚫 CI failed with log

@ghost
Copy link

ghost commented Sep 24, 2017

1 Warning
⚠️ Any source code changes should have an entry in CHANGELOG.md or have #trivial in their title.

Generated by 🚫 Danger

@rahul-malik rahul-malik merged commit c5ed50a into master Sep 24, 2017
@rahul-malik rahul-malik deleted the objc-reserved-keywords branch January 4, 2018 19:18
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

5 participants