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

Enable app download #69

Merged
merged 20 commits into from
Nov 2, 2017
Merged

Enable app download #69

merged 20 commits into from
Nov 2, 2017

Conversation

JoeSSS
Copy link
Contributor

@JoeSSS JoeSSS commented Oct 30, 2017

Fixes #70

@@ -333,6 +334,19 @@ class TasksViewController: NSViewController {
}
}

func downloadAppFromLink(link: String) {
if let launchPath = Constants.FilePaths.Bash.appDownload {
let outputStream = CommandsCore.CommandTextOutputStream()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the CommandsCore prefix here?

Also, can we please not add this to the VC?

someData.write(toFile: plistPath, atomically: true)
}

func readFromPlist(forKey: String) -> ([String], [String]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's rename this read

plistPath = documentDirectory.appending(defaultPlistPath)
}

func createPlist(data : [String : Any]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's rename this create

class CommandsController {
let applicationStateHandler = ApplicationStateHandler()

func downloadAppFromLink(link: String, textView: NSTextView) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I would pass in the CommandTextOutputStream or a textHsndler closure which you assign to the executor’s output stream instead of the text view.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created an issue for that: #74 , as I don't see any easy possibilities to do it here (we depend on printer class that require this view, so it will not make much sense to create this extra class if we do all the stuff in ViewController)

}

func create(data : [String : Any]) {
if !fileManager.fileExists(atPath: plistPath) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it not file exists. I think this would never have anything to remove when it enters the clause.
Why remove at all, can’t it just overwrite?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, it automatically overwrites. Thanks for pointing this out

plistDictionary = NSDictionary(contentsOfFile: plistPath)
}
if let objectsArray = plistDictionary?.mutableArrayValue(forKey: forKey) {
objectsArray.forEach { dictionary in
Copy link
Contributor

Choose a reason for hiding this comment

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

Can’t you reduce this and return a map of keys and values instead of the tuple of 2 arrays?

@JoeSSS JoeSSS added the feature label Nov 1, 2017
var deviceListIsEmpty = false
@objc dynamic var isRunning = false
let applicationStateHandler = ApplicationStateHandler()
let tagsController = TagsController()
var devices = [""]
var timer: Timer!
var pathToCalabashFolder = ""
var linkInfoArray: ([String], [String]) = ([], [])
Copy link
Contributor

Choose a reason for hiding this comment

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

I’d make it a map 🗺

@JoeSSS
Copy link
Contributor Author

JoeSSS commented Nov 1, 2017

@BasThomas @q231950 I think it looks better this way (see refactored PlistOperations)

@JoeSSS JoeSSS merged commit e1c0b84 into master Nov 2, 2017
@JoeSSS JoeSSS deleted the enable_app_download branch November 2, 2017 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants