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

Improve danger swift edit #94

Merged
merged 6 commits into from Nov 6, 2018
Merged

Improve danger swift edit #94

merged 6 commits into from Nov 6, 2018

Conversation

f-meloni
Copy link
Member

@f-meloni f-meloni commented Nov 5, 2018

This might be a little bit controversial in some points, i will try to put as much as possible details, images and inline comments about what i'm trying to do

PR Goal

screenshot 2018-11-05 at 19 43 40

If you run danger-swift edit you will see that, with no syntax highlighting, no auto competition.
The reason for that was that it was not correctly overriding the OTHER_SWIFT_FLAGS.
But once i did override them correctly (was enough remove the , let before = "-DXcode\"," to fix that) the syntax highlighting was correct, but it was not compiling.
In order to make it compile i had to link the danger library, this is why i though that was a good idea to actually use the --xcconfig-overrides option on the generate-xcodeproj command

This is the result

screenshot 2018-11-05 at 19 46 59

@@ -26,6 +26,12 @@ private final class DangerRunner {
logger.logInfo("Ran with: \(CommandLine.arguments.joined(separator: " "))")

let cliLength = CommandLine.arguments.count

guard cliLength - 2 > 0 else {
Copy link
Member Author

@f-meloni f-meloni Nov 5, 2018

Choose a reason for hiding this comment

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

Now is possible to run the code, but it was crashing

screenshot 2018-11-05 at 20 01 02

This is why i added a nice message that replaces the crash

@@ -65,72 +71,142 @@ public func Danger() -> DangerDSL {
return DangerRunner.shared.dsl
}

extension DangerDSL {
/// Fails on the Danger report
public var fails: [Violation] {
Copy link
Member Author

Choose a reason for hiding this comment

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

I added this to allow people that are using multiple files to use danger to post messages etc. with typing and auto complete, without have to import Danger in all the files (given you have a global instance of danger defined on the Dangerfile, and that the multiple files will be copied on the Dangerfile when danger is executed)
(It works also with the import anyway)

But in this way you get
screenshot 2018-11-05 at 19 48 07
without import Danger

@@ -51,6 +59,24 @@ extension Script {
let fileName = importPath.split(separator: "/").last
return folder.path + "Sources/\(name)/\(fileName ?? "")"
}

private func generateXCodeProjWithConfig(configPath: String) throws {
Copy link
Member Author

Choose a reason for hiding this comment

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

Would be better if we could pass to Marathon the xcconfig for the xcodeproj in the arguments, maybe worth propose that on the Marathon repo

@f-meloni f-meloni requested review from orta and sunshinejr and removed request for orta November 5, 2018 20:44
@orta
Copy link
Member

orta commented Nov 6, 2018

OK, yeah, I think this idea is 👍

@f-meloni f-meloni merged commit 04c5163 into danger:master Nov 6, 2018
@f-meloni f-meloni deleted the improve_danger_swift_edit branch November 6, 2018 15:40
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