I think a comment here might be useful as well. Initially I wanted to suggest renamingarg.Deprecated but I imagine that's a bigger change to roll out here. It's just not very nice to read this as something like

if arg.Deprecated.Valid && arg.IgnoreDeprecated && template.IsDeprecated

potentially the template struct could be refactored to haveIsDeprecated andDeprecatedMessage as opposed to just havingDeprecated message whose value being"some string" means the template is deprecated. Alternatively, the template could have a functionIsDeprecated?

I agree that the current expression witharg.Deprecated.Valid && arg.IgnoreDeprecated && template.Deprecated.Valid isn't the most readable.

Just for context: this method filters templates based on thearg param in the in-memory database used for testing. This logic ensures that only templates matching the requested deprecated state are returned.

TheDeprecated field is of typesql.NullBool, which is represented as:

type NullBool struct {Bool  boolValid bool // Valid is true if Bool is not NULL}

So we need to checkValid to avoid misinterpreting a NULL as false.

Thetemplate is of typedatabase.Template, generated bysqlc.
I could add a methodIsDeprecated to theTemplate struct to clean this up, but since that type is generated bysqlc, don't think it would be a good idea to add this type of custom logic directly at the DB layer. An alternative might be to define a wrapper around the generated type or introduce a view model in the service layer that handles this logic more cleanly but that may add more complexity than it solves, especially for a test-specific filter.

For readability I'd suggest a comment at most, or some light variable extraction.dbmem is only used for testing and it is planned to retire it at some point.

@cstyan added a comment infa8f999

Reading this change a little more closely, doesn't this invert the query logic?

Comparing with the SQL:

-- Filter by deprecatedAND CASEWHENsqlc.narg('deprecated') ::booleanIS NOT NULL THENCASEWHENsqlc.narg('deprecated') ::boolean THENdeprecated!=''ELSEdeprecated=''ENDELSE trueEND

Aside: I've found it useful to comment the Go code in dbmem with the relevant parts of the corresponding SQL query.

Yes, indbmem we are doing aninverse filtering to exclude non-matching templates.

In the SQL, we filter as follows:

• Ifdeprecated is set totrue, we only include templates wheredeprecated != '', i.e., there is a deprecation message.
• Ifdeprecated is set tofalse, we only include templates wheredeprecated = '', i.e., there is no deprecation message.
• Ifdeprecated is not set (NULL), we include all templates (ELSE true clause).

In the in-memory logic we are doing aninverse filtering, thecontinue is used to exclude non-matching templates:

if arg.Deprecated.Valid && arg.Deprecated.Bool != (template.Deprecated != "") {  continue}

We skip templates when theDeprecated filter is set and the template's deprecation statusdoes not match theDeprecated value. This mirrors the behavior of the SQL CASE logic.

I think what’s confusing here is the double negation. I’ll improve the comment and maybe add a simpleisDeprecated function for readability.

Introduced a helper functionisDeprecated and improved the comment by adding the corresponding SQL logic for better readability:06acc27
@cstyan@johnstcn let me know if this makes it clearer 🙂

👍 I like it

For readability I'd suggest a comment at most, or some light variable extraction.dbmem is only used for testing and it is planned to retire it at some point.