vapor-community / postgresql

Robust PostgreSQL interface for Swift

MIT License

131 stars 33 forks source link

Add support for LISTEN and NOTIFY #29

Closed johnbona closed 7 years ago

johnbona commented 7 years ago

I'm not all that comfortable with libpq so give this extra scrutiny. 🙊

I used this example heavily to help me determine how to receive notifications using libpq. I also purposefully broke convention and used a closure in listen instead of creating a onNotification property off Database since a database could be listening to multiple channels simultaneously.

Couple of things that I wanted to point out for feedback:

What to do with errors caught in listen since the closure in try background can't throw? Using fatalError right now but not sure if that's the correct approach.
The name of the Notification struct. It's not really meant to be used outside the listen closure so it's maybe okay to keep named as is even though it collides with Foundation's own Notification.

codecov-io commented 7 years ago

Codecov Report

Merging #29 into master will increase coverage by 0.03%. The diff coverage is 93.75%.

@@            Coverage Diff             @@
##           master      #29      +/-   ##
==========================================
+ Coverage   92.61%   92.65%   +0.03%     
==========================================
  Files          13       14       +1     
  Lines        1815     1879      +64     
==========================================
+ Hits         1681     1741      +60     
- Misses        134      138       +4

Impacted Files	Coverage Δ
Sources/PostgreSQL/Notification.swift	`100% <100%> (ø)`
Tests/PostgreSQLTests/PostgreSQLTests.swift	`96.23% <100%> (+0.19%)`	:arrow_up:
Sources/PostgreSQL/Database.swift	`84.61% <85.71%> (+2.79%)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update ff6bd68...fdb0d0b. Read the comment docs.

johnbona commented 7 years ago

@natebird Made the changes you requested and also found a slight performance increase using string concatenation instead of interpolation when constructing the SQL statements, especially for large NOTIFY payloads.

natebird commented 7 years ago

@tanner0101 @LoganWright - do you know why CircleCI isn't responding to PRs on this repo?

vzsg commented 7 years ago

Circle only responds to PRs which are started from this repo, not external forks. I don't know why it's so.

The typical workaround for me was to push the other branch here and start a new PR - you should be able to do this with your permissions.

natebird commented 7 years ago

@vzsg I figured out the issue. It is a setting in Circle. @johnbona can you push an empty commit to this branch again to kick Circle?

vzsg commented 7 years ago

Sorry, I couldn't resist :)

natebird commented 7 years ago

Yay!

natebird commented 7 years ago

@johnbona can you review this again? The tests are failing.

Now that we have a 2.0 Beta release for this repo we can merge this PR into the 2.0 release.

Thanks!

johnbona commented 7 years ago

@natebird Should be good now!