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

Add dataset validation for sync push command #241

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

DechengMa
Copy link
Contributor

@DechengMa DechengMa commented Oct 12, 2020

Adding validation and error handling logic for sync push command

📝 Summary of Changes

Changes proposed in this pull request:

  • Add duplicateTesters error for handling duplicate testers with the same email addresses.
  • Add noValidApp error when the user hasn't run sync pull before sync push

🧐🗒 Reviewer Notes

💁 Example

swift run asc testflight sync push

🔨 How To Test

swift run asc testflight sync push

@DechengMa DechengMa self-assigned this Oct 12, 2020
@DechengMa DechengMa added enhancement New feature or request regression A regression from previous behaviour labels Oct 12, 2020
@DechengMa DechengMa marked this pull request as ready for review October 12, 2020 23:24
@DechengMa DechengMa changed the title Adding data set validation for sync command Adding dataset validation for sync command Oct 12, 2020
@DechengMa DechengMa changed the title Adding dataset validation for sync command Adding dataset validation for sync push command Oct 12, 2020
@DechengMa DechengMa changed the title Adding dataset validation for sync push command Add dataset validation for sync push command Oct 12, 2020
@DechengMa DechengMa removed the regression A regression from previous behaviour label Oct 13, 2020
@@ -52,9 +52,20 @@ struct TestFlightProgramDifference {
}
}

enum Error: LocalizedError, Equatable {
Copy link
Contributor

Choose a reason for hiding this comment

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

There's an argument to be made around not adding equatable conformance in the implementation if you only use it for testing. Not sure how I feel about it though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's an argument to be made around not adding equatable conformance in the implementation if you only use it for testing. Not sure how I feel about it though

Any ideas for removing the Equatable in this case? 🤔

Copy link
Contributor

@adamjcampbell adamjcampbell Oct 14, 2020

Choose a reason for hiding this comment

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

If you wanted to you could replace the instances in testing with something like:

XCTAssertThrowsError(try TestFlightProgramDifference(local: local, remote: remote)) { error in
  guard case TestFlightProgramDifference.Error.duplicateTesters(let email) = error else {
    return XCTFail()
  }

  XCTAssertEqual(email, "foo@gmail.com")
}

@DechengMa DechengMa marked this pull request as draft November 30, 2020 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants