Templates #2400
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,150 @@ | ||
| // | ||
| // GithubClient+Template.swift | ||
| // Freetime | ||
| // | ||
| // Created by Ehud Adler on 11/11/18. | ||
| // Copyright © 2018 Ryan Nystrom. All rights reserved. | ||
| // | ||
|
|
||
| import Foundation | ||
| import GitHubAPI | ||
| import GitHubSession | ||
| import Squawk | ||
|
|
||
| private let githubIssueURL = ".github/ISSUE_TEMPLATE" | ||
|
|
||
|
|
||
| struct IssueTemplateRepoDetails { | ||
| let repo: RepositoryDetails | ||
| let defaultBranch: String | ||
| } | ||
|
|
||
| struct IssueTemplateDetails { | ||
| let repoDetails: IssueTemplateRepoDetails | ||
| let session: GitHubUserSession? | ||
| let viewController: UIViewController | ||
| weak var delegate: NewIssueTableViewControllerDelegate? | ||
|
There was a problem hiding this comment. Why are we passing around a delegate like this? It's coupling way too many things. Networking should just do networking, parsing should just do parsing. Instead we have a networker that is also showing alerts and presenting VCs. Too much! That work should be done by the calling VC in the completion block. If there's duplicated code, just create a Also could we just fetch the list of templates, show the alert (if there are >0 templates) w/ file names or something w/out fetching the actual contents? Then when the user selects a template fetch the contents. There's just wasted networking in here. There was a problem hiding this comment. @rnystrom We can't reduce the networking calls because the title and description of files are at the top of the files. The file names are not equal to the template names. Let me know if that makes sense. I will make the changes regarding passing the templates to the VC and letting it take care of displaying the alert. |
||
| } | ||
|
|
||
| extension GithubClient { | ||
|
|
||
| private func fetchTemplateFile( | ||
| repoDetails: IssueTemplateRepoDetails, | ||
| filename: String, | ||
| testingFile: String? = nil, | ||
| completion: @escaping (Result<String>) -> Void | ||
| ) { | ||
|
|
||
| // For Testing.. | ||
| if let testingFile = testingFile { completion(.success(testingFile)) } | ||
|
There was a problem hiding this comment. Let's newline the closure execution here to stay consistent w/ other |
||
|
|
||
| self.fetchFile( | ||
|
|
||
| owner: repoDetails.repo.owner, | ||
| repo: repoDetails.repo.name, | ||
| branch: repoDetails.defaultBranch, | ||
| path: "\(githubIssueURL)/\(filename)") { (result) in | ||
|
|
||
| switch result { | ||
| case .success(let file): completion(.success(file)) | ||
| case .error(let error): completion(.error(error)) | ||
| case .nonUTF8: completion(.error(nil)) | ||
|
There was a problem hiding this comment. nit: spacing inconsistent w/ rest of project. Newlines or single-space please |
||
| } | ||
| } | ||
| } | ||
|
|
||
| func createTemplate( | ||
|
There was a problem hiding this comment. nit: this needs a new name, I keep thinking that it's actually creating something. Can it just use the "fetch" naming used elsewhere? |
||
| repo: IssueTemplateRepoDetails, | ||
| filename: String, | ||
| testingFile: String? = nil, | ||
| completion: @escaping (Result<IssueTemplate>) -> Void | ||
| ) { | ||
| fetchTemplateFile(repoDetails: repo, filename: filename, testingFile: testingFile) { (result) in | ||
|
|
||
| switch result { | ||
| case .success(let file): | ||
| let nameAndDescription = IssueTemplateHelper.getNameAndDescription(fromTemplatefile: file) | ||
| if let name = nameAndDescription.name { | ||
| var cleanedFile = file | ||
|
|
||
| // Remove all template detail text | ||
| // ----- | ||
| // name: | ||
| // about: | ||
| // ----- | ||
| if let textToClean = file.matches(regex: "([-]{3,})([\\s\\S]*)([-]{3,})").first { | ||
|
There was a problem hiding this comment. Can we add individual tests for this? |
||
| if let range = file.range(of: textToClean) { | ||
| cleanedFile = file.replacingOccurrences( | ||
| of: textToClean, | ||
| with: "", | ||
| options: .literal, | ||
| range: range | ||
| ) | ||
| } | ||
| cleanedFile = cleanedFile.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| } | ||
| completion(.success(IssueTemplate(title: name, template: cleanedFile))) | ||
| } else { | ||
| completion(.error(nil)) | ||
| } | ||
| case .error(let error): | ||
| completion(.error(error)) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func createNewIssue( | ||
| repo: IssueTemplateRepoDetails, | ||
| session: GitHubUserSession?, | ||
| mainViewController: UIViewController, | ||
| delegate: NewIssueTableViewControllerDelegate, | ||
| completion: @escaping () -> Void | ||
| ) { | ||
|
|
||
| var templates: [IssueTemplate] = [] | ||
|
|
||
| // Create group. | ||
| // We need this since we will be making multiple async calls inside a for-loop. | ||
| let templateGroup = DispatchGroup() | ||
|
|
||
| let details = IssueTemplateDetails( | ||
| repoDetails: repo, | ||
| session: session, | ||
| viewController: mainViewController, | ||
| delegate: delegate | ||
| ) | ||
|
|
||
| self.fetchFiles( | ||
| owner: details.repoDetails.repo.owner, | ||
| repo: details.repoDetails.repo.name, | ||
| branch: details.repoDetails.defaultBranch, | ||
| path: githubIssueURL) { (result) in | ||
| switch result { | ||
| case .success(let files): | ||
| for file in files { | ||
| templateGroup.enter() | ||
| self.createTemplate(repo: repo, filename: file.name, completion: { (result) in | ||
| switch result { | ||
| case .success(let template): | ||
| templates.append(template) | ||
| case .error(let error): | ||
| if let err = error { Squawk.show(error: err) } | ||
|
There was a problem hiding this comment. VC should be showing error alerts. Also what happens if multiple things go wrong, we're showing multiple errors? Should just bubble up to a single error and let the VC handle it. |
||
| } | ||
| templateGroup.leave() | ||
| }) | ||
| } | ||
| case .error(let error): | ||
| if let err = error { Squawk.show(error: err) } | ||
|
|
||
| completion() | ||
| } | ||
|
|
||
| // Wait for async calls in for-loop to finish up | ||
| templateGroup.notify(queue: .main) { | ||
| completion() | ||
| // Sort lexicographically | ||
| let sortedTemplates = templates.sorted(by: {$0.title < $1.title }) | ||
| IssueTemplateHelper.present( | ||
| withTemplates: sortedTemplates, | ||
| details: details | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| // | ||
| // IssueTemplates.swift | ||
| // Freetime | ||
| // | ||
| // Created by Ehud Adler on 11/3/18. | ||
| // Copyright © 2018 Ryan Nystrom. All rights reserved. | ||
| // | ||
|
|
||
| import Foundation | ||
| import GitHubAPI | ||
| import GitHubSession | ||
| import Squawk | ||
|
|
||
| struct IssueTemplate { | ||
| let title: String | ||
| let template: String | ||
| } | ||
|
|
||
| class IssueTemplateHelper { | ||
|
|
||
|
|
||
| static func getNameAndDescription(fromTemplatefile file: String) -> (name: String?, about: String?) { | ||
|
|
||
| let names = file.matches(regex: String.getRegexForLine(after: "name")) | ||
| let abouts = file.matches(regex: String.getRegexForLine(after: "about")) | ||
| let name = names.first?.trimmingCharacters(in: .whitespaces) | ||
| let about = abouts.first?.trimmingCharacters(in: .whitespaces) | ||
| return (name, about) | ||
| } | ||
|
|
||
| static func showIssueAlert( | ||
| with templates: [IssueTemplate], | ||
| details: IssueTemplateDetails) -> UIAlertController { | ||
|
|
||
|
|
||
|
|
||
|
|
||
| let alertView = UIAlertController( | ||
| title: NSLocalizedString("New Issue", comment: ""), | ||
|
|
||
| message: NSLocalizedString("Choose Template", comment: ""), | ||
| preferredStyle: .actionSheet | ||
| ) | ||
|
|
||
| for template in templates { | ||
| alertView.addAction( | ||
| UIAlertAction( | ||
| title: template.title, | ||
| style: .default, | ||
| handler: { _ in | ||
| guard let viewController = NewIssueTableViewController.create( | ||
| client: GithubClient(userSession: details.session), | ||
| owner: details.repoDetails.repo.owner, | ||
| repo: details.repoDetails.repo.name, | ||
| template: template.template, | ||
| signature: details.repoDetails.repo.owner == Constants.GitHawk.owner ? .bugReport : .sentWithGitHawk | ||
| ) else { | ||
| assertionFailure("Failed to create NewIssueTableViewController") | ||
| return | ||
| } | ||
| viewController.delegate = details.delegate | ||
| let navController = UINavigationController(rootViewController: viewController) | ||
| navController.modalPresentationStyle = .formSheet | ||
| details.viewController.route_present(to: navController) | ||
|
There was a problem hiding this comment. router handles wrapping in nav controller There was a problem hiding this comment. @rnystrom Can you explain this a bit? If I switch to There was a problem hiding this comment. Also a separate question: Should it be route_detail? on iPad it pushes the new VC over the entire screen, not into the detail section There was a problem hiding this comment. I see now that route_details wraps a nav controller if necessary. Cool stuff. Ill make this switch. |
||
| })) | ||
| } | ||
|
|
||
| alertView.addAction( | ||
| UIAlertAction( | ||
| title: NSLocalizedString("Dismiss", comment: ""), | ||
| style: .cancel, | ||
| handler: { _ in | ||
| alertView.dismiss(animated: trueUnlessReduceMotionEnabled) | ||
| }) | ||
| ) | ||
| return alertView | ||
| } | ||
|
|
||
| static func present( | ||
| withTemplates sortedTemplates: [IssueTemplate], | ||
| details: IssueTemplateDetails) { | ||
| if sortedTemplates.count > 0 { | ||
| // Templates exists... | ||
| var templates = sortedTemplates | ||
| templates.append( | ||
| IssueTemplate( | ||
| title: NSLocalizedString("Regular Issue", comment: ""), | ||
| template: "" | ||
| ) | ||
| ) | ||
| let alertView = IssueTemplateHelper.showIssueAlert( | ||
| with: templates, | ||
| details: details | ||
| ) | ||
| details.viewController.route_present(to: alertView) | ||
| } else { | ||
| // No templates exists, show blank new issue view controller | ||
| guard let viewController = NewIssueTableViewController.create( | ||
| client: GithubClient(userSession: details.session), | ||
| owner: details.repoDetails.repo.owner, | ||
| repo: details.repoDetails.repo.name, | ||
| signature: .sentWithGitHawk | ||
| ) else { | ||
| assertionFailure("Failed to create NewIssueTableViewController") | ||
| return | ||
| } | ||
| viewController.delegate = details.delegate | ||
| let navController = UINavigationController(rootViewController: viewController) | ||
| navController.modalPresentationStyle = .formSheet | ||
| details.viewController.route_present(to: navController) | ||
|
There was a problem hiding this comment. same note about nav controller, router handles wrapping |
||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra space