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

Remove sourcery #78

Merged
merged 5 commits into from Jul 10, 2018

Conversation

Projects
None yet
2 participants
@yhkaplan
Copy link
Contributor

yhkaplan commented Jul 7, 2018

What

As per #76, this removes Sourcery templates and code in favor of Swift 4.1 automatic Equatable conformance.

Why

Code generation for Equatable is not necessary anymore as Swift 4.1 features automatic conformance for Equatable. While Sourcery is reliable, using native Swift auto-conformance makes for a simpler toolset when getting started with Danger, and is significantly less likely to require maintenance between major Swift versions.

How

  • Remove templates, sourcery.yml, AutoEquatable protocol, and conformance declarations
  • Add Equatable with Xcode's regex find and replace as below

2018-07-07 12 39 13

Concerns

  • Adding Equatable conformance in the Test target did not work, meaning it had to be added directly in the Main target
    • This adds a small amount of clutter to the model objects affected
  • Some model objects feature an "ID" or otherwise unique property that is technically the only parameter needed to define equatability
    • These model objects may have this more technically "correct" conformance added, causing tests to miss all the other parameters
    • For now, I believe this doesn't present a major problem, but it may cause tests coverage to drop dramatically if not watched
    • One solution may be to write a Danger rule that's triggered by func == (rhs: ...), reminding the developer that the tests require a workaround
@yhkaplan

This comment has been minimized.

Copy link
Contributor

yhkaplan commented Jul 7, 2018

CI failing at Danger JS, but I don't understand what "Danger JS did not include fails" means. Perhaps the Dangerfile has some kind of error?

Error:  Error: Results passed to Danger JS did not include fails.
{}
    at /Users/travis/.nvm/versions/node/v6.12.3/lib/node_modules/danger/distribution/dsl/DangerResults.js:20:19
    at Array.forEach (native)
    at Object.validateResults (/Users/travis/.nvm/versions/node/v6.12.3/lib/node_modules/danger/distribution/dsl/DangerResults.js:16:24)
    at Executor.<anonymous> (/Users/travis/.nvm/versions/node/v6.12.3/lib/node_modules/danger/distribution/runner/Executor.js:162:41)
    at step (/Users/travis/.nvm/versions/node/v6.12.3/lib/node_modules/danger/distribution/runner/Executor.js:32:23)
    at Object.next (/Users/travis/.nvm/versions/node/v6.12.3/lib/node_modules/danger/distribution/runner/Executor.js:13:53)
    at /Users/travis/.nvm/versions/node/v6.12.3/lib/node_modules/danger/distribution/runner/Executor.js:7:71
    at __awaiter (/Users/travis/.nvm/versions/node/v6.12.3/lib/node_modules/danger/distribution/runner/Executor.js:3:12)
    at Executor.handleResults (/Users/travis/.nvm/versions/node/v6.12.3/lib/node_modules/danger/distribution/runner/Executor.js:158:16)
    at Object.<anonymous> (/Users/travis/.nvm/versions/node/v6.12.3/lib/node_modules/danger/distribution/commands/utils/runDangerSubprocess.js:108:47)
The command "DEBUG="*" danger process .build/x86_64-apple-macosx10.10/debug/danger-swift" exited with 1.
@orta

This comment has been minimized.

Copy link
Member

orta commented Jul 8, 2018

This feels like a danger-js bug tbh

@yhkaplan

This comment has been minimized.

Copy link
Contributor

yhkaplan commented Jul 9, 2018

Seems to be ok on the Linux CI and my local machine

@orta

This comment has been minimized.

Copy link
Member

orta commented Jul 10, 2018

I shipped an update with danger-js for some improved logging, but I don't think that should block this PR

@orta orta merged commit d159a14 into danger:master Jul 10, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@yhkaplan yhkaplan deleted the yhkaplan:remove-sourcery branch Jul 11, 2018

@peril-staging

This comment has been minimized.

Copy link
Contributor

peril-staging bot commented Jul 22, 2018

Thanks for the PR @yhkaplan.

This PR has been shipped in v0.4.1 - CHANGELOG.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment