This repository has been archived by the owner on Aug 14, 2019. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Tiebout - Apple Push Notification Server App #1084
Tiebout - Apple Push Notification Server App #1084
Changes from all commits
f7bfef7
00fe3eb
ac66d9a
cf0b409
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-1 for old syntax It's in a bunch of other places too.
These two really should be
=*
anyway, it's specifically made for putting different/easier faces on things.Do we not need to check for
red
to actually be bigger than what's currently stored fornom
? I don't think so, but just checking.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you include type information in
=*
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! Since #585 you can do
=* foo=bar baz
. I actually forget this made it in already, and should've been using it more. Neat!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we want to re-subscribe from the last-heard message going forward? I think the logic below ensures we don't send notifications for whatever old junk we may get here, but still.
Pretty sure this just gives you the last
100
messages, or all messages from the last~d1
or something along those lines. Being more specific allows you to catch up from even further back when needed, and doesn't get you more info than you need.There was a problem hiding this comment.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to always produce a list of either zero moves, or one move. Sounds like a
(unit move)
to me.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the code style of sending back a list much more. I'd need to add another layer of conditional checks in diff-hall-rumor otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I guess
(list move)
is still fair game here. The contract between "notification" and "moves" is a bit weird, so maybe comment, but approved either way.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could do with a type for
pay
.my
isn't commonly used,~(gas in *(map @t json))
is more verbose/clear on what it does (and type-checks early!). For this case, you could also~(put by *(map @t json))
since you only have one element.Or just do the dumbest thing and say
[alert+s+'etc' ~ ~]
and cast that to amap
. (^:(The most aggressive thing to do here, of course, is to tell @pilfer-pandex to put "map/set syntax" on the global hoon wishlist.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is certainly on my radar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also wait a second,
'cords'
don't support string interpolation! You need to(crip "tape {etc}")
here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like gas in map. It's less clear to me than my.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, so kings route your notifications for you? Is this because we don't want any old ship to access our push endpoint? idk about loading it all onto ~dopzod, we may want a dedicated ship for this, but the load probably isn't that big?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|^
exists and might be nicer here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nest-failed in a weird way, skipping it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to help you figure it out if you want to, but I won't press you on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want a
@tas
from the@t
p.jon
, you can either just cast it, or?> (sane %tas p.jon)
and then cast, or similar.(scot %tas
just gives you the "rendered" version, which may not be what you want.