Skip to content

Conversation

realaravinth
Copy link
Contributor

It is not a coma separated value. Add tests and fix the documentation.


This PR originates from @dachary of the forgefriends project. Please see here for origin.

It is not a coma separated value. Add tests and fix the documentation.
@realaravinth realaravinth marked this pull request as draft June 10, 2022 12:56
@realaravinth realaravinth marked this pull request as ready for review June 10, 2022 13:55
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jun 12, 2022
@Gusted Gusted added this to the 1.17.0 milestone Jun 12, 2022
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 12, 2022
"pull_requests",
"comments",
}

Copy link
Contributor

@wxiaoguang wxiaoguang Jun 12, 2022

Choose a reason for hiding this comment

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

I think this default value is not ideal for the server code.

The default value should be 0-length slice to indicate all units (there may be more in the future).

update: if it guarantees that the units always contains the units to be restored, the support for len(units) == 0 should be deleted to make the logic consistent.

image

Copy link
Member

Choose a reason for hiding this comment

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

I think it's a good idea to reduce the possibility to forget the places when we support more units.

Usage: `Which items will be restored, one or more units should be separated as comma.
wiki, issues, labels, releases, release_assets, milestones, pull_requests, comments are allowed. Empty means all units.`,
Value: &defaultUnits,
Usage: "Which items will be restored, can be repeated",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Usage: "Which items will be restored, can be repeated",
Usage: "Which items (wiki, issues, labels, releases, release_assets, milestones, pull_requests, comments) will be restored, can be repeated. Not providing this flag means all units.",

@wxiaoguang
Copy link
Contributor

Since it works, no blocker from my side. Just put my thoughts here.

@realaravinth
Copy link
Contributor Author

This pull request is closed as requested by the author who will not be able to answer further questions here because they do not have a GitHub account.

@lunny lunny removed this from the 1.17.0 milestone Jun 12, 2022
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants