refactor: small optimize for sql query #940
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
appleboy
changed the title
lunny
added
the
type/refactoring
|
If it compiles and runs you have my LGTM :) |
tboerger
added
the
lgtm/need 1
lunny
requested changes
Feb 15, 2017
models/repo.go
Outdated
| if err = repos[i].GetOwner(); err != nil { | ||
| return nil, err | ||
| } | ||
| if err = repos.loadAttributes(x); err != nil { |
There was a problem hiding this comment.
But maybe sometime, we don't need to loadAttributes?
There was a problem hiding this comment.
Yes you're right. Ony two condition need to loadAttributes.
Searcheris not nil.- Star page of user profile
There was a problem hiding this comment.
I prefer add a new field IfLoadAttributes bool for SearchRepoOptions. Then we can not hard code there.
There was a problem hiding this comment.
I forgot one place need to update.
See new commit: aa9fac1
We don't need a new field IfLoadAttributes, just two condition need GetOwner data:
opts.Searcher != nil || opts.Starred
models/repo.go
Outdated
| if err = repos.loadAttributes(x); err != nil { | ||
| return nil, 0, fmt.Errorf("LoadAttributes: %v", err) | ||
| } | ||
| if err = repos.loadAttributes(x); err != nil { |
appleboy
force-pushed
the
sql3
branch
2 times, most recently
from
105f4c3 to
974dea2
Compare
lunny
approved these changes
Feb 15, 2017
|
LGTM |
tboerger
added
lgtm/done
1 task
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
RepositoriesWithUsers.repo.GetOwnerloop condition.cc @lunny