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

Improved installation process #5

Closed
wants to merge 7 commits into from
Closed

Improved installation process #5

wants to merge 7 commits into from

Conversation

chrisozenne
Copy link

  • CouchDBSample's main.swift now creates a connection on port 5984, the default port used by CouchDB upon install.
  • Changed the name of the default database which Kitura tries to connect to: kitura_test_db. The README.md is also updated to mention that a database with this name needs to be created.
  • The README.md has updated and more thorough installation instructions.
  • Improved error logging with HeliumLogger.

… default port used by CouchDB upon install.

Changed the name of the default database which Kitura tries to connect to: `kitura_test_db`. The README.md is also updated to mention that a database with this name needs to be created.
The README.md has updated and more thorough installation instructions.
- Changed from `class` to `struct`
- All properties are non-optional, reducing the need to check for nil.
- `hostName` is now just `host`.
- `userName` is now `username`.
- `HTTPProtocol` is now a computed property.
- `URL` is now a computed property. (and now contains an @ sign between the password and host)
- `toString()` has been replaced with a computed `description` property conforming to the `CustomStringConvertible` protocol.

`CouchDBUtils.swift`:
- Removed some nil-checking code for properties which are no longer optional.
- Updated code to property name changes in other files.

`main.swift`:
- Updated code to property name changes in other files.
- Instead of creating a JSON string literal, then converting it into data and then into a JSON (SwiftyJSON) object, we're now just simply creating a SwiftyJSON object literal and it's much easier to read.
- Added a few // MARK: lines for organization.
- Reorganized the document functions into CRUD order, create, read, update then delete. (With a // MARK: for each)
- In general, formatting matches that of default Xcode formatting (4 spaces instead of 2 for indenting).
@dfirsht
Copy link
Contributor

dfirsht commented Mar 21, 2016

@shmuelk Please review this so it can be merged.

@shmuelk
Copy link
Collaborator

shmuelk commented Mar 21, 2016

The change to ConnectionProperties, making userid and password required fields with the changes that followed, will cause the API to fail on a server that doesn't have security enabled.

Please change that part back to the way it was.

The rest is very good.
Thanks for your efforts in improving our CouchDB APIs

@chrisozenne
Copy link
Author

@shmuelk Can you point me to where this failure is happening so that I can better understand the issue?

@shmuelk
Copy link
Collaborator

shmuelk commented Mar 21, 2016

The URL created by:

// CouchDB URL
var URL: String {
return "(HTTPProtocol)://(username):(password)@(host):(port)"
}

Will only work iff there actually is a userid and password. If I am running a local server, I may not want to activate security, yet you are forcing it. The old code build the URL differently depending on whether or not there was a userid and password.

…trings once again.

Instead: a warning will be logged if a ConnectionProperties instance is initialized without supplying a username and password.
No longer parsing runtime arguments for connection properties and no longer requiring a username and password among those arguments.
@chrisozenne
Copy link
Author

@shmuelk Please review latest changes. Thanks!

@shmuelk
Copy link
Collaborator

shmuelk commented Mar 21, 2016

A couple of questions on the latest changes to: CouchDBSample:

  1. Why did you remove the userid and password parameters? They should be made optional.
  2. Why did you hard code the host instead of getting it as a parameter?

I think the number of parameters should be either one or three (i.e. always with a host and with or without a userid and password pair).

Otherwise everything is fine.

@chrisozenne
Copy link
Author

From this comment:

// Parse runtime args... this is just an interim solution

It appeared that the previous runtime arguments were just a temporary solution. There was no mention of what they were a temporary solution to, so if there is a strong reason for accepting runtime arguments then please let me know and I can change that part back.

As an outsider, it's not 100% clear to me what the blueprint is, so some more documentation or comments would be very much appreciated so that I don't accidentally break some un-documented requirement.

Thanks.

@chrisozenne
Copy link
Author

@shmuelk can you help me get this merged?

@shmuelk
Copy link
Collaborator

shmuelk commented Apr 20, 2016

Merged into develop in the commit 6c17251

@shmuelk shmuelk closed this Apr 20, 2016
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.

None yet

3 participants