add subcomands for notifications #283

Closed
khmarbaise wants to merge 7 commits from khmarbaise/tea:issue-243-add_subcomands_for_notifications into master
Collaborator

Fix #243

Signed-off-by: Karl Heinz Marbaise kama@soebes.de

Fix #243 Signed-off-by: Karl Heinz Marbaise <kama@soebes.de>
khmarbaise added 1 commit 1 year ago
ce4d69ece9
add subcomands for notifications
khmarbaise reviewed 1 year ago
Dismissed
// Copyright 2020 The Gitea Authors. All rights reserved.
// Use of this source code is governed by a MIT-style
// license that can be found in the LICENSE file.
Poster
Collaborator

Is this the way to go with common code from cmd/... ? Or should that put into cmd/notifications/ ?

Is this the way to go with common code from cmd/... ? Or should that put into cmd/notifications/ ?
noerw marked this conversation as resolved
noerw added the
kind/feature
label 1 year ago
khmarbaise added 1 commit 1 year ago
8ce46fe569
Added missing comment on exported function.
khmarbaise added 1 commit 1 year ago
eb8fd52023
Fixed exported function comments.
noerw requested changes 1 year ago
Dismissed
Name: "ls",
Aliases: []string{"list"},
Usage: "List notifications....",
Description: `List notification....`,
noerw commented 1 year ago
Collaborator
- 	Usage:       "List notifications....",
+ 	Usage:       "List notifications",
```diff - Usage: "List notifications....", + Usage: "List notifications", ```
noerw marked this conversation as resolved
Description: `List notification....`,
Action: RunNotificationsList,
Flags: append([]cli.Flag{
&cli.BoolFlag{
noerw commented 1 year ago
Collaborator

Please move these common flags into cmd/flags/flags.go as NotificationFlags

Please move these common flags into `cmd/flags/flags.go` as `NotificationFlags`
Poster
Collaborator

Good idea not even realized that. Thanks.

Good idea not even realized that. Thanks.
noerw marked this conversation as resolved
&cli.BoolFlag{
Name: "all",
Aliases: []string{"a"},
Usage: "show all notifications of related gitea instance",
noerw commented 1 year ago
Collaborator
- 			Usage:   "show all notifications of related gitea instance",
+ 			Usage:   "Show notifications across all your repos instead of the current repo only",
```diff - Usage: "show all notifications of related gitea instance", + Usage: "Show notifications across all your repos instead of the current repo only", ```
noerw marked this conversation as resolved
},
&cli.StringFlag{
Name: "state",
Usage: "Filter by milestone state (all|open|closed)",
noerw commented 1 year ago
Collaborator
  • Are valid states really called all/open/closed? all/read/unread?
  • Doesn't this flag duplicate functionality from the subcommands read/unread?
  • s/milestone/notification
- Are valid states really called all/open/closed? all/read/unread? - Doesn't this flag duplicate functionality from the subcommands read/unread? - s/milestone/notification
Poster
Collaborator

Yes of course.

Yes of course.
noerw marked this conversation as resolved
// CmdNotificationsPinned represents a sub command of notifications to list pinned notifications
var CmdNotificationsPinned = cli.Command{
Name: "pinned",
Aliases: []string{"pin"},
noerw commented 1 year ago
Collaborator

"pin" makes it sound like a verb, not a read list-command.
I'd drop this alias

"pin" makes it sound like a verb, not a read list-command. I'd drop this alias
Poster
Collaborator

What about with an alias pd instead?

What about with an alias `pd` instead?
noerw marked this conversation as resolved
listOpts := flags.GetListOptions(ctx)
if listOpts.Page == 0 {
listOpts.Page = 1
}
noerw commented 1 year ago
Collaborator

@6453 🤔

@6453 🤔
Poster
Collaborator

If you take a look into flags.GetListOptions(ctx)

func GetListOptions(ctx *cli.Context) gitea.ListOptions {
	page := ctx.Int("page")
	limit := ctx.Int("limit")
	if limit != 0 && page == 0 {
		page = 1
	}
	return gitea.ListOptions{
		Page:     page,
		PageSize: limit,
	}
}
If you take a look into `flags.GetListOptions(ctx)` ``` func GetListOptions(ctx *cli.Context) gitea.ListOptions { page := ctx.Int("page") limit := ctx.Int("limit") if limit != 0 && page == 0 { page = 1 } return gitea.ListOptions{ Page: page, PageSize: limit, } } ```
Poster
Collaborator

After I've played a little bit with it which means removed the lines from notification_list.go I've not seen a difference. May I have missed a thing... Can someone enlighten me ?

After I've played a little bit with it which means removed the lines from notification_list.go I've not seen a difference. May I have missed a thing... Can someone enlighten me ?
noerw commented 1 year ago
Collaborator

I don't understand why, but these lines are needed still, without I get no results for tea notif read

I don't understand why, but these lines are needed still, without I get no results for `tea notif read`
6543 commented 1 year ago
Collaborator

in short: it enforces paggination

in short: it enforces paggination
6543 marked this conversation as resolved
khmarbaise added 1 commit 1 year ago
29212a5d3e
Integrated review comments.
khmarbaise requested review from noerw 1 year ago
noerw approved these changes 1 year ago
Dismissed
6543 reviewed 1 year ago
Dismissed
6543 requested changes 1 year ago
Dismissed
Name: "all",
Aliases: []string{"a"},
Usage: "show all notifications of related gitea instance",
},
6543 commented 1 year ago
Collaborator

@khmarbaise can you move task.ListNotifications back to cmd/notification module

I dont see a reason to add cli & flag dependency into task module

@khmarbaise can you move task.ListNotifications back to cmd/notification module I dont see a reason to add **cli** & **flag** dependency into task module
Poster
Collaborator

So making a file notifications_list.go which contains that. I will do and change it accordingly.

So making a file `notifications_list.go` which contains that. I will do and change it accordingly.
Poster
Collaborator

Done as suggested.

Done as suggested.
khmarbaise marked this conversation as resolved
khmarbaise added 1 commit 1 year ago
d350600d81
Moved notifications_list from task into cmd/notifications
khmarbaise requested review from 6543 1 year ago
khmarbaise force-pushed issue-243-add_subcomands_for_notifications from d350600d81 to 85c3001e49 1 year ago
6543 reviewed 12 months ago
Dismissed
print.NotificationsList(news, flags.GlobalOutputValue, ctx.Bool("all"))
func runNotificationsDetails(ctx *cli.Context) error {
log.Fatal("Not yet implemented.")
6543 commented 12 months ago
Collaborator

I would print use --help to see all options

I would print `use --help to see all options`
6543 reviewed 12 months ago
Dismissed
Collaborator

I dont get the usage of pin, unpin, read subcommand

they only list stuff as list does?

I expect pin to actualy pin a notification ...

same vor unpin & read

the list should print out the notif id to this actions specific or with or,

  • --all flag for read/pin mark all unread as read
  • --all flag for unpin mark all pined as read

unread should only be used to mark a specific one as "unread"

I dont get the usage of pin, unpin, read subcommand they only list stuff as `list` does? I expect pin to actualy pin a notification ... same vor unpin & read the list should print out the notif id to this actions specific or with or, - `--all` flag for read/pin mark all unread as read - `--all` flag for unpin mark all pined as read unread should only be used to mark a specific one as "unread"
Collaborator

@khmarbaise can you resolve conflicts?

If you need help just shout out on discord to me

@khmarbaise can you resolve conflicts? If you need help just shout out on discord to me
6543 added this to the v0.7.0 milestone 12 months ago
noerw added the
status/needs-feedback
label 12 months ago
khmarbaise force-pushed issue-243-add_subcomands_for_notifications from 85c3001e49 to abd32a7074 12 months ago
6543 modified the milestone from v0.7.0 to v0.8.0 9 months ago
Collaborator

-> #386

-> #386
6543 closed this pull request 4 months ago
6543 referenced this issue from a commit 3 months ago
Some checks failed
continuous-integration/drone/pr Build is failing
Required
Details
Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Loading…
There is no content yet.