[go: up one dir, main page]

Skip to content

Settings sync follow-up: Convert setting_type column from text to enum

The following discussion from !134571 (merged) should be addressed:

  • @Alexand started a discussion: (+3 comments)

    suggestion / question

    I'm sorry, @ealcantara. 🤦 But I only realized this now, and I think it's relevant to the current change. We could think about this also in a follow-up, but it might mean more reworking in the future.

    Have we considered having the setting_type to be an enum? It feels like it's the perfect case for an enum as the values are predefined by the upstream VS Code API specification and the users don't determine the text that goes there, right?

    If this is the case, I think we should consider making this attribute an enum in Rails, and a smallint in the database table. See https://docs.gitlab.com/ee/development/database/creating_enums.html.

    The smallint will make the unique index smaller, the table data used smaller, and we don't need this check constraint anymore: CONSTRAINT check_994c503fc4 CHECK ((char_length(setting_type) <= 256)).

    Although, this would mean that you'd need to do some more changes to your code here. Do we have a good reason to not have an enum, but a text column instead?

    We can also push this question to the database maintainer to weigh-in if you prefer.

    In practice this change would mean:

    1. Changing the column type.
    2. Creating an enum out of SETTINGS_TYPES, which goes in line with that idea of bringing it to the model instead of the service library.
    3. So there might be a small refactor to be done on the code where you currently call SETTINGS_TYPES too, so to reference the new enum keys instead of the constant hash array. You can still just move that constant to the model and assign the enum like: enum setting_type: SETTING_TYPES, but it needs to become a hash with numbers as values. Then I think you can call SETTING_TYPES.keys if you want to get an array with value names.