Add subcomands for notifications #386

Closed
6543 wants to merge 18 commits from 6543:add-subcomands-for-notifications_rebase283 into master
6543 commented 4 months ago
Collaborator

Fix #243

use #283 and resolved conflicts + some code dedub

Fix #243 use #283 and resolved conflicts + some code dedub
6543 added 9 commits 4 months ago
6543 added 1 commit 4 months ago
0f12c0a77f
Next dedub
6543 added the
kind/feature
status/wip
labels 4 months ago
6543 added 1 commit 4 months ago
noerw requested changes 4 months ago
noerw left a comment

Initial review, still quite some work to do (I might pick it up this or next week, no promises though)

// NotificationFlags defines flags that should be available on notifications.
var NotificationFlags = append([]cli.Flag{
&cli.BoolFlag{
Name: "all",
noerw commented 4 months ago
Collaborator

can we rename this to user, account or any-scope?
all is ambiguous as it could also mean --state=all #usability

can we rename this to `user`, `account` or `any-scope`? `all` is ambiguous as it could also mean `--state=all` #usability
Name: "all",
Aliases: []string{"a"},
Usage: "show all notifications of related gitea instance",
Description: "Show notifications, by default based of the current repo",
noerw commented 4 months ago
Collaborator

Description: "Show notifications, by default based on the current repo if available",

` Description: "Show notifications, by default based on the current repo if available",`
Flags: append(flags.NotificationFlags,
&cli.StringFlag{
Name: "state",
Usage: "set notification state (default is all), pinned,read,unread",
noerw commented 4 months ago
Collaborator
- set notification...
+ filter by notification...
```diff - set notification... + filter by notification... ```
// notif ls --state all
// notif ls --state pinned
// notif ls --state read
// notif ls --state unread
noerw commented 4 months ago
Collaborator

?

?
ctx := context.InitCommand(cmd)
client := ctx.Login.Client()
all := ctx.Bool("all")
noerw commented 4 months ago
Collaborator

see above

see [above](https://gitea.com/gitea/tea/pulls/386#issuecomment-443162)
//This enforces pagination.
listOpts := ctx.GetListOptions()
if listOpts.Page == 0 {
listOpts.Page = 1
noerw commented 4 months ago
Collaborator

do we really want this here? a reusable helper would be better. or even move this logic into the sdk?

do we really want this here? a reusable helper would be better. or even move this logic into the sdk?
Status: status,
})
} else {
ctx.Ensure(context.CtxRequirement{RemoteRepo: true})
noerw commented 4 months ago
Collaborator

this is a breaking change, previously tea n ran fine without a remote repo, and just listed the user notifications.
I think the new behaviour is better, so we just need to make sure to note that breaking change in the PR description/labels

this is a breaking change, previously `tea n` ran fine without a remote repo, and just listed the user notifications. I think the new behaviour is better, so we just need to make sure to note that breaking change in the PR description/labels
// CmdNotificationsPinned represents a sub command of notifications to list pinned notifications
var CmdNotificationsPinned = cli.Command{
Name: "pinned",
noerw commented 4 months ago
Collaborator

These subcommands are unneeded/wrong (was a misunderstanding with the original author i think):
We want subcommands that pin/unpin + mark as read/unread.

These subcommands are unneeded/wrong (was a misunderstanding with the original author i think): We want subcommands that pin/unpin + mark as read/unread.
noerw added this to the v0.8.0 milestone 4 months ago
noerw added 7 commits 3 months ago
5ee01374bf
refactor FieldsFlag as CsvFlag for reusability
4acda8d111
rename --for-user flag to --mine for consistency
8414c23ed5
print notification id
Collaborator

superseded by #389

superseded by #389
noerw closed this pull request 3 months ago
noerw deleted branch add-subcomands-for-notifications_rebase283 3 months ago
6543 referenced this issue from a commit 3 months ago

Reviewers

noerw requested changes 4 months ago
Some checks failed
continuous-integration/drone/pr Build is failing
Required
Details
This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Loading…
There is no content yet.