The great refactor -- Refactor config to YAML, Add Docs, Revise exported fields. about photoprism

[lastzero]

Concerning YAML config reader: I figured it is not really needed at this point, as the environment is configured via Docker env variables. Might be needed if installed without Docker (that's how we started). So I just left it like it is.

[lastzero]

Config should use functions, not fields, to return values. It's on my personal to do list. Again, there should be good test coverage first.

[Skarlso]

What do you mean functions instead of fields? I mean, Go is simple. It doesn't very much like getters. :) It's not Java.

Things like these are not idiomatic:

type MyStruct struct{
    Field1 string
}

func (m MyStruct) GetField1() string {
    return m.Field1
}

These are not things you want in Go. :)

[Skarlso]

Also, even if it's environment properties, that config part where you load in the file and then have a bunch of ifs defining the variables if they exists, it's just like using a marshaller in the end. They'll be nil if nothing is defined in the file.

Hmm, that reminds me. I should set omitempty. :)

[lastzero]

For example, I started doing this so that not every single sub-directory needs to be defined in the config file (c.AssetsPath should be changed to c.assetsPath when refactoring is done):

func (c *Config) GetAssetsPath() string {
	return c.AssetsPath
}

func (c *Config) GetTensorFlowModelPath() string {
	return c.GetAssetsPath() + "/tensorflow"
}

So now of course you could argue that all those paths could be properly populated upfront and the functions are not needed.

[lastzero]

You suggest directly populating config via yaml.Unmarshal()? Is that a good idea, assuming you might want at least a little bit of abstraction and validation between the file and what you use internally? I see you can do it like that, I just want to think about it before we go that way (if that's what you suggest).

[Skarlso]

So, Go isn't like that. Go is a very simplistic, C style language, with limited OOP capabilities. It love what's simple. Marshalling works in huge projects and in Go itself and it various other situations without making a getter setter. Getter setters are a reminiscence of Java, PHP and other languages who promote OOP. Go is more like C in this regard. As simple as possible don't think about it too much. :)

In the above example you provide GetAssetsPath would be a good thing only if it's doing something before accessing the property. Like, validation, or default setting, or something else. In this form is absolutely useless and clutters the code needlessly.

Let me ask you this. What would the abstraction be good for? What would it actually add in this situation? What do you expect to get out of a layer of indirection like that?

If you want an abstraction just to have an abstraction you don't need the abstraction. :)

[lastzero]

For me, Go is Oberon that looks like C. Using functions is not OOP.

The reason why I started to refactor this is that some config values had in fact be returned by functions like GetTensorFlowModelPath(), unless I want to put every single sub-directory in the config file (which also means the config file would change a lot and become big, fat and ugly).

As a developer, I want to get all config values the same way and I don't want to investigate if a particular value needs abstraction or not. That's simplicity.

[Skarlso]

So, you have one case where having a function is okay, because you are also setting something while getting the data. But then that's not a getter any more that's a function providing some information and a possible side effect as an extra. :)

The rest, is still simply returning a field. Which doesn't require a function at all and imho only convolutes the code. Reminds me of long forgotten days of Java Beans shudders in horror.

Thus, by that logic, if your config happens to have 20 fields, you'd write 20 functions for each field? :)

[lastzero]

Before we use this ticket to start a discussion about Java Beans, here are two examples from the actual code:

func (c *Config) GetPublicBuildPath() string {
	return c.GetPublicPath() + "/build"
}

func (c *Config) GetDb() *gorm.DB {
	if c.db == nil {
		c.ConnectToDatabase()
	}

	return c.db
}

[Skarlso]

Example for what? GetPublicPath there does not need to be a function. That's the only thing I'm talking about here. The rest is fine not counting the fact that they don't have to be exported. If I remember correctly neither of them are used outside this package. Or maybe dB does. But that's not the point here.

All im saying is that unless you modify the data there is no need for the function. If you modify the data it's no longer a getter but a method that provides extra information.

[lastzero]

All im saying is that unless you modify the data there is no need for the function. If you modify the data it's no longer a getter but a method that provides extra information.

Exactly the kind of detail I don't want to care about when I use the data (in this case paths) somewhere else in the code. This might change and then you need to do a lot of refactoring just to save a function here and now.

Simplicity also lies within a uniform interface and abstraction of implementation details. The same logic was used against Pascal by the C crowd who preferred to not have strings in return for a little bit of performance. Every C developer now has to care about the implementation details of strings and there are a lot of security issues related to it and the manual memory management. That's not simplicity.

If this makes it more logical: The config paths might be dynamic later, if we build a multi-user version. I don't want the code to assume they are constant.

[Skarlso]

Okay then. 😊👌

[lastzero]

Good that we talked about it. This is the kind of information that is valuable as inline comment or in the developer guide.

[lastzero]

@Skarlso Closing this as there have not been more contributions. Thank you for the pull requests and the great advice! 👍

I've created a page in our Wiki related to this: Code Quality

[Skarlso]

@lastzero So sorry man. I got completely swamped. :/ Once I can pick up the threads of my life again, I'll reopen and work on this some more. :) Happy to have helped out though! :)

[lastzero]

@Skarlso Happy new year and thank you for the feedback ⭐️ You're welcome back anytime!