Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[API] Milestone endpoints accept names too #12649

Merged
merged 14 commits into from
Sep 14, 2020

Conversation

6543
Copy link
Member

@6543 6543 commented Aug 30, 2020

similar to #12366 but for milestones

is very usefull for https://gitea.com/gitea/go-sdk/pulls/388 ... and so on

@codecov-commenter
Copy link

codecov-commenter commented Aug 30, 2020

Codecov Report

Merging #12649 into master will increase coverage by 0.01%.
The diff coverage is 48.27%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12649      +/-   ##
==========================================
+ Coverage   43.15%   43.17%   +0.01%     
==========================================
  Files         654      654              
  Lines       72205    72218      +13     
==========================================
+ Hits        31159    31179      +20     
+ Misses      35998    35986      -12     
- Partials     5048     5053       +5     
Impacted Files Coverage Δ
modules/indexer/code/bleve.go 71.16% <ø> (-0.27%) ⬇️
routers/api/v1/repo/milestone.go 57.69% <46.15%> (+3.84%) ⬆️
modules/indexer/code/indexer.go 35.95% <66.66%> (+0.72%) ⬆️
modules/indexer/stats/db.go 52.17% <0.00%> (-8.70%) ⬇️
modules/queue/unique_queue_disk_channel.go 53.84% <0.00%> (-1.54%) ⬇️
modules/git/repo.go 49.74% <0.00%> (-0.51%) ⬇️
services/pull/pull.go 41.57% <0.00%> (-0.47%) ⬇️
models/gpg_key.go 55.40% <0.00%> (+0.58%) ⬆️
routers/repo/view.go 38.11% <0.00%> (+0.64%) ⬆️
modules/log/event.go 59.43% <0.00%> (+1.88%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 33f606c...51ebde6. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 30, 2020
@6543
Copy link
Member Author

6543 commented Sep 1, 2020

as per @lafriks lable kind/breanking needed

@techknowlogick techknowlogick added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Sep 1, 2020
@techknowlogick techknowlogick added this to the 1.13.0 milestone Sep 1, 2020
@6543
Copy link
Member Author

6543 commented Sep 2, 2020

@zeripath renamed
But If I remove ctx from this function, I'll have to handle Errors in a seperat function (if dont like to create duplicated code) witch relay on ctx then

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Sep 2, 2020
@CirnoT
Copy link
Contributor

CirnoT commented Sep 4, 2020

I really wonder if this is necessary to implement; the API does not need to act via name for user convenience because it's not designed for that, and CLI utility can implement a very simple logic to fetch ID and names mappings from API. This would also allow CLI to perform some interesting resolve mechanism, such as:

  • 1 -> ID
  • "1" -> name
  • kind/breaking -> name
  • "kind/breaking" -> name

What do you say @6543, wouldn't this be better abstracted from main Gitea code?

@6543
Copy link
Member Author

6543 commented Sep 4, 2020

@CirnoT "resolving" the milestone by name was called "low efficient" so I go this way around
and indeed will this end up in one api call instead of 2

@techknowlogick techknowlogick added the modifies/api This PR adds API routes or modifies them label Sep 4, 2020
@CirnoT
Copy link
Contributor

CirnoT commented Sep 5, 2020

It might be easiest, but are we really going to support this for every single API that takes ID?

@6543
Copy link
Member Author

6543 commented Sep 5, 2020

@CirnoT otherwise we have to do this on client side:

// ResolveMilestoneIDByName is a fallback method to find milestone id by name
func (c *Client) ResolveMilestoneIDByName(owner, repo, name string) (int64, error) {
	i := 0
	for {
		i++
		miles, err := c.ListRepoMilestones(owner, repo, ListMilestoneOption{
			ListOptions: ListOptions{
				Page: i,
			},
			State: "all",
			Name: name,
		})
		if err != nil {
			return 0, err
		}
		if len(miles) == 0 {
			return 0, fmt.Errorf("milestone '%s' do not exist", name)
		}
		for _, m := range miles {
			if strings.ToLower(strings.TrimSpace(m.Title)) == strings.ToLower(strings.TrimSpace(name)) {
				return m.ID, nil
			}
		}
	}
}

@6543
Copy link
Member Author

6543 commented Sep 5, 2020

It might be easiest, but are we really going to support this for every single API that takes ID?

@CirnoT only if it realy make sence (to lower api requests) I would say

@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 Sep 14, 2020
@lafriks
Copy link
Member

lafriks commented Sep 14, 2020

🚀

@lafriks lafriks merged commit 00a806d into go-gitea:master Sep 14, 2020
@lafriks lafriks deleted the api-milestones-accept-names branch September 14, 2020 11:48
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
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. modifies/api This PR adds API routes or modifies them pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants