Skip to content

Conversation

@dabear
Copy link
Contributor

@dabear dabear commented Jul 1, 2017

This adds an option to select US and NON_US share servers in the GUI. Also, if the customServer variable is filled out, a third option for selecting "Custom" share server will appear in the GUI

Copy link
Collaborator

@ps2 ps2 left a comment

Choose a reason for hiding this comment

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

Looking good!

options: [
(title: NSLocalizedString("US", comment: "U.S. share server option title"),
value: DexcomShareURL.absoluteString)
value: DexcomShareURL.absoluteString),
Copy link
Collaborator

Choose a reason for hiding this comment

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

DexcomShareURL should be replaced everywhere with the new string constants from Mark’s lib

if let username = username, let password = password, url != nil {
isAuthorized = true
client = ShareClient(username: username, password: password)
client = ShareClient(username: username, password: password, shareServer: url!.absoluteString)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should avoid use of ! with let url = url, in the if statement above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, agreed. Can you explain why the url != nil was added in the first place (rather than let url = url)..?

Copy link
Collaborator

Choose a reason for hiding this comment

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

url wasn't used at that point; it would not compile with a let (would be an unused variable), so it was just a hint.

}

let client = ShareClient(username: username, password: password)
let client = ShareClient(username: username, password: password, shareServer: url!.absoluteString)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Avoid !

)
]

if customServer.characters.count > 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Using nullable/optional var here would be better than counting chars.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started doing that, but realized having an optional set to an initial state of nil would be more difficult to grasp for novice users not accustomed to editing code, instead of editing just the contents between two quotation marks.

I guess i could use an optional with a more extensive description

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or maybe don't even set a string at all in the no-custom-server case; just have a block of code that is completely commented out.


init(username: String?, password: String?, url: URL?) {
// change the contents of the variable below to point to your custom share server
let customServer = ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Feel free to expand upon how to do this; like linking to your repo/blog. Also, do you want to describe how to change the name as shown in the UI from "Custom"?

@dabear
Copy link
Contributor Author

dabear commented Jul 2, 2017

The requested changes should now been added. Please verify :)

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.

2 participants