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

Implement the API endpoint for listing releases #116

Merged
merged 5 commits into from
Nov 30, 2020
Merged

Implement the API endpoint for listing releases #116

merged 5 commits into from
Nov 30, 2020

Conversation

417-72KI
Copy link
Contributor

Implements the API endpoint to list releases on a GitHub repository.

Also fixes some properties which will be null when the draft release is returned from API.

Comment on lines +59 to +61
func listReleases(_ session: RequestKitURLSession = URLSession.shared, owner: String, repository: String, completion: @escaping (_ response: Response<[Release]>) -> Void) -> URLSessionDataTaskProtocol? {
let router = ReleaseRouter.listReleases(configuration, owner, repository)
return router.load(session, dateDecodingStrategy: .formatted(Time.rfc3339DateFormatter), expectedResultType: [Release].self) { releases, error in
Copy link
Contributor

Choose a reason for hiding this comment

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

Diffs aren't automatically wrapped everywhere, so please avoid long lines, as it makes the code harder to review on devices without a wide screen. 100-120 characters line limit should be sufficient in most cases.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that we should limit the lines to some amount of characters. I don’t know if 100-120 is the correct number but it should be enforced with some swiftformat/swiftlint rules so it applies everywhere for everyone.

I personally wouldn’t leave this as a mandatory change to get this PR accepted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm aware of this one, and SwiftFormat recently also gained an ability to fit code into given line length too 🙂

("testPostRelease", testPostRelease),
("testLinuxTestSuiteIncludesAllTests", testLinuxTestSuiteIncludesAllTests)
]

// MARK: Actual Request tests

func testListReleases() {
let session = OctoKitURLTestSession(expectedURL: "https://api.github.com/repos/octocat/Hello-World/releases", expectedHTTPMethod: "GET", jsonFile: "Fixtures/releases", statusCode: 200)
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: please split this to multiple lines to reduce the length of this line for easier review.

@417-72KI
Copy link
Contributor Author

@MaxDesiatov
Thank you for reviewing!

I'm ready to split long lines, but if I do that, it will be different style from other lines.
Is it acceptable?

@MaxDesiatov
Copy link
Contributor

Maybe some formatting rules applied with SwiftFormat could be introduced in a separate PR? I'll defer to @AvdLee on this matter

OctoKit/Releases.swift Show resolved Hide resolved
OctoKit/Releases.swift Show resolved Hide resolved
@pietbrauer pietbrauer merged commit bf37dd1 into nerdishbynature:master Nov 30, 2020
@pietbrauer
Copy link
Member

Thanks a lot!

@417-72KI 417-72KI deleted the list-releases branch December 1, 2020 03:51
@417-72KI 417-72KI mentioned this pull request Dec 18, 2020
tngranados pushed a commit to tngranados/octokit.swift that referenced this pull request Jul 13, 2022
Implement the API endpoint for listing releases
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.

4 participants