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

Avoid including "num_issues" and "num_closed_issues" multiple times #8337

Closed

Conversation

yzzyx
Copy link
Contributor

@yzzyx yzzyx commented Oct 1, 2019

Only update columns that have not been specified already in the query.

Fixes issue #8336.

@codecov-io
Copy link

Codecov Report

Merging #8337 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8337      +/-   ##
==========================================
+ Coverage   41.62%   41.63%   +<.01%     
==========================================
  Files         495      495              
  Lines       65523    65524       +1     
==========================================
+ Hits        27274    27280       +6     
+ Misses      34730    34725       -5     
  Partials     3519     3519
Impacted Files Coverage Δ
models/issue_label.go 58.56% <100%> (+0.14%) ⬆️
models/repo_indexer.go 68.21% <0%> (+1.93%) ⬆️

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 177aedf...1d33dd2. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 1, 2019
Copy link
Member

@guillep2k guillep2k left a comment

Choose a reason for hiding this comment

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

It looks like this should be taken into account by xorm, but it doesn't hurt handling it here as well.

@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 Oct 1, 2019
@lafriks
Copy link
Member

lafriks commented Oct 1, 2019

This should be fixed upstream in xorm (cc: @lunny ) as with this it will be harder to keep list updated if new columns are added to that table etc

@lunny
Copy link
Member

lunny commented Oct 2, 2019

I have sent go-xorm/xorm#1446, please wait for that merged. It's merged and we need a new PR on gitea to update xorm.

@guillep2k
Copy link
Member

I have sent go-xorm/xorm#1446, please wait for that merged. It's merged and we need a new PR on gitea to update xorm.

@lunny we've recently adopted the tagged xorm 0.7.8. Should we reference master again?
Also, #8317 is already obsolete.

@sapk
Copy link
Member

sapk commented Oct 2, 2019

@guillep2k I merged #8317 as it still clean the double dependency and I think it is better to do it step by step. Just need to re-do the same steps like any update deps 😄

  • tag xorm
  • update xormstore
  • update gitea xorm and xormstore deps (if possible in one PR)

@guillep2k
Copy link
Member

@guillep2k I merged #8317 as it still clean the double dependency and I think it is better to do it step by step. Just need to re-do the same steps like any update deps 😄

* tag xorm

* update xormstore

* update gitea xorm and xormstore deps (if possible in one PR)

@sapk It makes sense. Let's wait for xorm to be tagged, then. Please you make the PR when that's ready.

@lunny
Copy link
Member

lunny commented Oct 2, 2019

I have tag xorm v0.7.9

@guillep2k
Copy link
Member

I have tag xorm v0.7.9

@sapk Do you want to take care of the PR?

@sapk
Copy link
Member

sapk commented Oct 2, 2019

@guillep2k please go if you have time. I will maybe have time tonight but don't wait on me if you can do it.

@guillep2k
Copy link
Member

@guillep2k please go if you have time. I will maybe have time tonight but don't wait on me if you can do it.

@sapk Oh, I thought you wanted to make a single PR containing both changes. I can do the Gitea part of course.

@yzzyx
Copy link
Contributor Author

yzzyx commented Oct 3, 2019

Fixed upstream in xorm, and with PR #8355, closing this pr.

@yzzyx yzzyx closed this Oct 3, 2019
@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/need 1 This PR needs approval from one additional maintainer to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants