`RepoSpec` specified in both `Notification` struct and `ExternalService` interface func arguments. github.com/shurcooL/notificationsapp#6

Issues doesn't set RepoSpec in the struct. Should it be removed? Or is it meant for the return values of InternalService.List. If so we should probably make a NotificationSpec struct.

Yes, this is a case of one general Notification struct representing a "notification". When creating one via Notify endpoint, not all need to be set.

I think the solution to this is similar to the change in apps/tracker@0ddb0fa5. Notice how I was previously (ab)using the actual Comment struct in EditComment method, and changed it so that EditComment has a dedicated CommentRequest struct instead.

Similarly, we should change signature of Notify to:

Notify(ctx context.Context, appID string, repo issues.RepoSpec, threadID uint64, nr NotificationRequest) error

// NotificationRequest contains fields needed to specify a notification.
type NotificationRequest {
	Title   string
	Icon    OcticonID
	HTMLURL template.URL // Address of notification target.
}

It's a minor enhancement that I've put off until later, but it can/should be done.

@keegancsmith, does that make sense?

Do you want to keep this thread open to track the change above, or shall we just close it since it's a pretty clear future enhancement plan? We can keep it open if you think it's valuable for others.

Edit: I may have accidentally double posted, sorry.

Sorry, I remember reading this but didn't respond. Yes that makes a lot of sense. Lets just close this out, and when we play around with the API we can discuss it then.

It's a minor enhancement that I've put off until later, but it can/should be done.

This has been done now.

A dedicated NotificationRequest struct that represents a request to create a notification was created in https://github.com/shurcooL/notifications/commit/53fb0f395d02cd2d2f5ba4ca00ee8cbc0af43c51, which resolves this issue.