ostafen
commented
2 years ago Hi, @segfault99, thank you for working on this. Regarding the choice of putting the code inside the normalize() function, I think that this is no so good, because we mix unrelated responsibilities inside a single function. Actually, the normalize() function has a well defined concern: it is responsible for converting a given value to one of internal clover types. So I would leave it untouched (if you have a look at the feature/gob branch, the normalize function is put under the encoding module, so we would also end with a circular dependency between modules if we add the *Document additional parameter).
What I think should be done is to introduce an helper function like this:
// getFieldOrValue() returns the result of dereferencing the field if value is a *field,
// otherwise it returns the input value itself
normValue := normalize(getFieldOrValue(doc, value))If you want to only use a single function, we could also introduce a new function which calls getFieldOrValue() and normalize() in sequence. The important part is that we don't give to the normalize() function a responsibility it is not supposed to have.
What do you think? Do you agree?
Moreover, since I have made several important changes inside the feature/gob branch, it would be better to start from that branch for implementing this, in order to avoid conflicts
I have added the field dereferencing code into the normalize function in order to not change code structure much.