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

Move user related model into models/user #17781

Merged
merged 7 commits into from
Nov 24, 2021
Merged

Conversation

lunny
Copy link
Member

@lunny lunny commented Nov 23, 2021

As title.

@lunny lunny added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Nov 23, 2021
@lunny lunny force-pushed the lunny/model_user3 branch 3 times, most recently from 568eb75 to f5fde1f Compare November 24, 2021 04:42
@wxiaoguang
Copy link
Contributor

Is models/db/name.go in the right package?

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 24, 2021
@lunny
Copy link
Member Author

lunny commented Nov 24, 2021

Is models/db/name.go in the right package?

The variables and functions in that files will be shared for other models/user and future models/repo packages. Or we have to create a new sub package to do that.

@wxiaoguang
Copy link
Contributor

OK, then we can have another PR to continue the refactoring, this PR is already so large.

LGTM

@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 Nov 24, 2021
@@ -22,7 +23,6 @@ type LFSLock struct {
ID int64 `xorm:"pk autoincr"`
Repo *Repository `xorm:"-"`
RepoID int64 `xorm:"INDEX NOT NULL"`
Owner *User `xorm:"-"`
Copy link
Member

Choose a reason for hiding this comment

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

Is the linter able to detect unused members?

Copy link
Contributor

Choose a reason for hiding this comment

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

The linter is able to detect unused private members, but exported one like these aren't detected.

I think the linter is preventing to detect this, as this Field could be used when go-gitea/gitea is imported as package and a golang program is using that field. But given that go-gitea/gitea shouldn't be used as package(right?), we can modify the deadcode linter and be more aggressive and also check on these Public fields etc. Not sure which refactor PR it was mentioned, but we also could check if a certain function is only used with the xxx_test.go packages and move it to them.

https://github.com/golangci/go-misc/blob/master/deadcode/deadcode.go

Copy link
Member

Choose a reason for hiding this comment

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

Currently we expect no one imports gitea. Otherwise this change would not be possible as we break the "api".

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, I will open up a issue for such proposal. Should I open up a issue at gitea-vet or just here.

@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 Nov 24, 2021
@lunny
Copy link
Member Author

lunny commented Nov 24, 2021

make L-G-T-M work

@lunny lunny merged commit a666829 into go-gitea:main Nov 24, 2021
@lunny lunny deleted the lunny/model_user3 branch November 24, 2021 09:49
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
* Move user related model into models/user

* Fix lint for windows

* Fix windows lint

* Fix windows lint

* Move some tests in models

* Merge
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
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/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants