modules/setting probably needs refactoring #1764

New Issue

GiteaMirror · 2025-11-02T04:12:36-06:00

GiteaMirror commented

2025-11-02 04:12:36 -06:00

Originally created by @mikolysz on GitHub (May 7, 2018).

Gitea version (or commit ref): 0b718e0d7b

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.

Originally created by @mikolysz on GitHub (May 7, 2018). 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.

GiteaMirror added the issue/confirmed type/refactoring labels 2025-11-02 04:12:36 -06:00

GiteaMirror closed this issue

2025-11-02 04:12:36 -06:00

GiteaMirror commented

2025-11-02 04:12:37 -06:00

@lunny commented on GitHub (May 7, 2018):

@devil418 Please try that.

@lunny commented on GitHub (May 7, 2018): @devil418 Please try that.

GiteaMirror commented

2025-11-02 04:12:37 -06:00

@lafriks commented on GitHub (May 8, 2018):

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.

@lafriks commented on GitHub (May 8, 2018): 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.

GiteaMirror commented

2025-11-02 04:12:37 -06:00

@bkcsoft commented on GitHub (Jul 8, 2018):

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 { ... }

@bkcsoft commented on GitHub (Jul 8, 2018): 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: ```go // ssh.go func (s *SSH) Parse(section *ini.Section) error { ... } // setting.go // func NewContext... ssh := SSH{} if err := ssh.Parse(); err != nil { ... } ```

GiteaMirror commented

2025-11-02 04:12:37 -06:00

@stale[bot] commented on GitHub (Jan 19, 2019):

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.

@stale[bot] commented on GitHub (Jan 19, 2019): 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.

GiteaMirror commented

2025-11-02 04:12:38 -06:00

@lunny commented on GitHub (Jan 19, 2019):

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

@lunny commented on GitHub (Jan 19, 2019): Like @bkcsoft 's idea, don't close.

GiteaMirror commented

2025-11-02 04:12:38 -06:00

@stale[bot] commented on GitHub (Mar 20, 2019):

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.

@stale[bot] commented on GitHub (Mar 20, 2019): 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.

GiteaMirror commented

2025-11-02 04:12:38 -06:00

@delvh commented on GitHub (Apr 29, 2023):

How up to date is this issue?
Can it be closed by now? The package has changed drastically…

@delvh commented on GitHub (Apr 29, 2023): How up to date is this issue? Can it be closed by now? The package has changed drastically…

GiteaMirror referenced this issue

2025-11-02 12:00:56 -06:00

[PR #1764] [MERGED] Update HighlightJS and fix YAML files highlighting #16041

Sign in to join this conversation.