-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Go: set subtypes
column to true for models where it has a meaning
#17966
base: main
Are you sure you want to change the base?
Conversation
This is almost always what we want.
I'd say we might as well leave this set to False where we're modelling a function, not a method, if only to avoid confusing a reader wondering what a "subtype" means in that context |
I was thinking it was easier to have the convention that we always set it to True unless there was a very good reason to, so we don't have to think about what is a function and what is a method. Admittedly it's pretty obvious from whether the "type" column is empty or not, but I still think some people will set it to False for methods by mistake at some point if we mix True and False. |
QA showed 3 extra alerts. I was able to reproduce one locally and it was a valid result. I wasn't able to reproduce the other two locally. I don't think it's worth chasing it down too much - the small number of extra alerts reassures me that this PR is okay to merge. |
Regarding the discussion about what to set this to for functions, my view is:
|
@mbg 's higher-minded suggestions notwithstanding, more people seem to be in favour of having subtypes=False for rows where it makes no sense (which is precisely when the types column is empty). I have pushed a commit making the change. For the record there are 553 rows with the types column empty and 827 rows with the types column non-empty. |
subtypes
column to true for all models (except in some tests)subtypes
column to true for models where it has a meaning
This is almost always the behaviour that we want. I will do a QA run to see what alert changes this leads to.