Skip to content
This repository has been archived by the owner on Aug 14, 2019. It is now read-only.

Tiebout - Apple Push Notification Server App #1084

Merged
merged 4 commits into from
Mar 1, 2019
Merged

Conversation

tacryt-socryp
Copy link
Contributor

Simple gall app for sending push notifications to an urbit user's iPhone when they receive a Hall message from a circle that has been subscribed to these notifications.

@vvisigoth
Copy link
Contributor

@loganallenc remember to request review (I just added @ixv and @Fang- )

Copy link
Member

@Fang- Fang- left a comment

Choose a reason for hiding this comment

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

Bunch of style things, some logic things. Big review all in all. @loganallenc, apologies and good luck. (;

lib/tiebout.hoon Outdated Show resolved Hide resolved
lib/tiebout.hoon Outdated Show resolved Hide resolved
lib/tiebout.hoon Outdated Show resolved Hide resolved
lib/tiebout.hoon Outdated Show resolved Hide resolved
app/tiebout.hoon Show resolved Hide resolved
mar/tiebout-action.hoon Outdated Show resolved Hide resolved
app/tiebout.hoon Outdated Show resolved Hide resolved
app/tiebout.hoon Show resolved Hide resolved
app/tiebout.hoon Outdated Show resolved Hide resolved
app/tiebout.hoon Show resolved Hide resolved
Copy link
Member

@Fang- Fang- left a comment

Choose a reason for hiding this comment

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

Thanks for the touch-ups! Here's a few stragglers.

The /lib file still exists, that needs to be thrown out of version control.

Lastly, there's still old syntax and indentation oddities around. (?+s for the most part.) If you do one last style pass over this, you should be good. (:

app/tiebout.hoon Outdated
[~ this]
?~ wir
(mean [leaf+"invalid wire for diff: {(spud wir)}"]~)
?+ i.wir
Copy link
Member

Choose a reason for hiding this comment

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

I thought I left a comment about this in the initial review but can't find it, so maybe it got eaten? You already do a saner ?+ in the ++diff-hall-* cases, you'll want to apply that here and in ++quit too.

(Also indenting for this particular one is borked.)

app/tiebout.hoon Outdated
=/ not/notification :+
token.sta
'com.tlon.urbit-client'
pay
Copy link
Member

Choose a reason for hiding this comment

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

You're probably fine putting these between [] on one line. If you feel that's too long, proper :+ form would be

:+  token.sta
  'com.tlon.urbit-client'
pay

sur/tiebout.hoon Outdated
@@ -0,0 +1,62 @@
::
:::: /lib/tiebout/hoon
Copy link
Member

Choose a reason for hiding this comment

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

Pretty sure filepaths in file headers is outdated style.

And as mentioned before, lots of the structures (cards, state) here really belong in the /app file instead, since they aren't shared to/used by other applications/libraries interfacing with Tiebout.

@tacryt-socryp
Copy link
Contributor Author

Hey man! I removed the lib from the repository, made the indentation change and removed the ?~ i.wir you've mentioned. I also added the change to the +: for the notification that you mentioned. This should be ready to go now. Thanks!

@tacryt-socryp tacryt-socryp requested a review from Fang- March 1, 2019 01:09
Copy link
Member

@Fang- Fang- left a comment

Choose a reason for hiding this comment

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

This should be ready to go now.

There's still old syntax in there. (;
My comment about string interpolation in 'cords' (line 244) also remains unresolved.

These aren't show-stoppers per se, I guess. Assuming you tested this to actually work, yeah, this can go in.

Thanks for bearing with me. (^:

::
:: %our
::
{%our @ @}
Copy link
Member

Choose a reason for hiding this comment

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

While these are technically correct and will successfully match /our/whatever aka [%our %whatever ~], it's better to describe a limited-length wire by actually ending with ~ in the type description.

@tacryt-socryp tacryt-socryp merged commit efc1873 into next Mar 1, 2019
@tacryt-socryp tacryt-socryp deleted the tiebout-next branch March 1, 2019 18:37
@ixv ixv mentioned this pull request Mar 7, 2019
joemfb added a commit that referenced this pull request Mar 13, 2019
* master: (38 commits)
  bumped ames protocol number
  landscape 3f83c798bd61b7e6cef5c4e2f7c7f3ac89d4ec09
  removed hard calls on json blobs
  Support eth_getBlockByNumber Ethereum RPC call
  added initial image support to udon parser (#1085)
  Tiebout - Apple Push Notification Server App (#1084)
  Implement +from-unix for turning timestamps into @da
  Add support to `lens-command` for pill output and optimized base64 encoding. (#1068)
  Point to the correct topics when decoding Azimuth events
  Add trailing newlines
  Use unit to disambiguate poll timer state
  Lightly re-order ++watcher core, add comments
  Implement ++peek so the app can by scried
  Remove debug pokes
  Implement %eth-watcher, an app for tracking Ethereum events
  add control flow to |verb
  Be accurate in incoming/hoon-side structures also
  More accurately represent Ethereum RPC filter topics
  also pin validate-x to now
  pin to local time because using local desk
  ...
BernardoDeLaPlaz pushed a commit to BernardoDeLaPlaz/arvo that referenced this pull request Mar 22, 2019
* App for sending Apple Push Notifications

* First pass at Hall subscription logic

* Tiebout app works end to end, can receive actions via Eyre, and can resubscribe to circles

* style changes for tiebout
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants