Gitea: modules/setting probably needs refactoring

[devil418]

Gitea version (or commit ref): 0b718e0d7b0fe4eb11121700df6c476e0f6b7e6a

Description

The setting module has the following problems and bad code smells:

• [ ] improper english in some comments and names (i.e. // enumerates all the policy repository creating, IsRunUserMatchCurrentUser)
• [ ] Very long functions (NewContext is currently 399! lines long).
• [ ] Non-idiomatic names that don't reflect what the code's doing (maybe they reflect what the code was doing earlier or what it was supposed to be doing)? Those include - Functions Named NewXXX which don't return any new instances of anything, setting global variables instead. Names like ConfigureXXX, SetXXX or SetupXXX would be more appropriate.
• [ ] Some structs declared as type literals should be declared with normal type Declarations to avoid repeating the code (see Repository.Editor)
• [ ] Some code is mindlessly copypasted without even being looket at. The message Error saving generated JWT Secret to custom config: %v", is repeated twice (and for a third time elsewhere, there might be more), and it appears in places completely not related to the saving of the jwt secret. The problem is the lack of a "saveCustomConfig" function and very similar code sprinkled through the module and other parts of the codebase (cmd/web.go is a good example).
• [ ] Some variables and the code that sets them aren't close, particularly those set by NewContext (because the function is huge, breaking it down into smaller functions and breaking the big var block into many smaller ones, declared close to the related functions would probably help).
• [ ] This is a bit of personal taste, but I believe that the ordering of the functions should be a bit different. Go is not c, and declaring functions (like DateLang, getAppPath etc.) before their usage makes the code less clear. For someone reading the file top to bottom, it's more useful to see the functions used in context and then declared instead of doing it the other way around.

I'm willing to work on this in the next few weeks if the maintainers are ok with that. I'd like to get feedback on which points to fix and which ones to leave as they are.

[lunny]

@devil418 Please try that.

[lafriks]

Please feel free to work on that. Also it is better to submit multiple small PR than single large as that helps to get it merged faster as it is easier to review.

[bkcsoft]

I would start by breaking out all structs to it's own file (like modules/setting/ssh.go) with their own Parse function that gets called.

Kind of like this:

// ssh.go
func (s *SSH) Parse(section *ini.Section) error { ... }

// setting.go
// func NewContext...
ssh := SSH{}
if err := ssh.Parse(); err != nil { ... }

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 weeks. Thank you for your contributions.

[lunny]

Like @bkcsoft 's idea, don't close.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 weeks. Thank you for your contributions.