Skip to content
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

Add Mexican Spanish as a project language #3588

Merged

Conversation

@benjaoming
Copy link
Contributor

@benjaoming benjaoming commented Feb 8, 2018

It should be possible to select as a distinct alternative to Spanish.

image

Copy link
Contributor

@agjohnson agjohnson left a comment

Thanks for the update! A new migration is required for this addition, could you update this PR to move the alter to a new migration?

@@ -63,7 +63,7 @@ class Migration(migrations.Migration):
('django_packages_url', models.CharField(max_length=255, verbose_name='Django Packages URL', blank=True)),
('privacy_level', models.CharField(default=b'public', help_text='(Beta) Level of privacy that you want on the repository. Protected means public but not in listings.', max_length=20, verbose_name='Privacy Level', choices=[(b'public', 'Public'), (b'protected', 'Protected'), (b'private', 'Private')])),
('version_privacy_level', models.CharField(default=b'public', help_text='(Beta) Default level of privacy you want on built versions of documentation.', max_length=20, verbose_name='Version Privacy Level', choices=[(b'public', 'Public'), (b'protected', 'Protected'), (b'private', 'Private')])),
('language', models.CharField(default=b'en', help_text="The language the project documentation is rendered in. Note: this affects your project's URL.", max_length=20, verbose_name='Language', choices=[(b'aa', b'Afar'), (b'ab', b'Abkhaz'), (b'af', b'Afrikaans'), (b'am', b'Amharic'), (b'ar', b'Arabic'), (b'as', b'Assamese'), (b'ay', b'Aymara'), (b'az', b'Azerbaijani'), (b'ba', b'Bashkir'), (b'be', b'Belarusian'), (b'bg', b'Bulgarian'), (b'bh', b'Bihari'), (b'bi', b'Bislama'), (b'bn', b'Bengali'), (b'bo', b'Tibetan'), (b'br', b'Breton'), (b'ca', b'Catalan'), (b'co', b'Corsican'), (b'cs', b'Czech'), (b'cy', b'Welsh'), (b'da', b'Danish'), (b'de', b'German'), (b'dz', b'Dzongkha'), (b'el', b'Greek'), (b'en', b'English'), (b'eo', b'Esperanto'), (b'es', b'Spanish'), (b'et', b'Estonian'), (b'eu', b'Basque'), (b'fa', b'Iranian'), (b'fi', b'Finnish'), (b'fj', b'Fijian'), (b'fo', b'Faroese'), (b'fr', b'French'), (b'fy', b'Western Frisian'), (b'ga', b'Irish'), (b'gd', b'Scottish Gaelic'), (b'gl', b'Galician'), (b'gn', b'Guarani'), (b'gu', b'Gujarati'), (b'ha', b'Hausa'), (b'hi', b'Hindi'), (b'he', b'Hebrew'), (b'hr', b'Croatian'), (b'hu', b'Hungarian'), (b'hy', b'Armenian'), (b'ia', b'Interlingua'), (b'id', b'Indonesian'), (b'ie', b'Interlingue'), (b'ik', b'Inupiaq'), (b'is', b'Icelandic'), (b'it', b'Italian'), (b'iu', b'Inuktitut'), (b'ja', b'Japanese'), (b'jv', b'Javanese'), (b'ka', b'Georgian'), (b'kk', b'Kazakh'), (b'kl', b'Kalaallisut'), (b'km', b'Khmer'), (b'kn', b'Kannada'), (b'ko', b'Korean'), (b'ks', b'Kashmiri'), (b'ku', b'Kurdish'), (b'ky', b'Kyrgyz'), (b'la', b'Latin'), (b'ln', b'Lingala'), (b'lo', b'Lao'), (b'lt', b'Lithuanian'), (b'lv', b'Latvian'), (b'mg', b'Malagasy'), (b'mi', b'Maori'), (b'mk', b'Macedonian'), (b'ml', b'Malayalam'), (b'mn', b'Mongolian'), (b'mr', b'Marathi'), (b'ms', b'Malay'), (b'mt', b'Maltese'), (b'my', b'Burmese'), (b'na', b'Nauru'), (b'ne', b'Nepali'), (b'nl', b'Dutch'), (b'no', b'Norwegian'), (b'oc', b'Occitan'), (b'om', b'Oromo'), (b'or', b'Oriya'), (b'pa', b'Panjabi'), (b'pl', b'Polish'), (b'ps', b'Pashto'), (b'pt', b'Portuguese'), (b'qu', b'Quechua'), (b'rm', b'Romansh'), (b'rn', b'Kirundi'), (b'ro', b'Romanian'), (b'ru', b'Russian'), (b'rw', b'Kinyarwanda'), (b'sa', b'Sanskrit'), (b'sd', b'Sindhi'), (b'sg', b'Sango'), (b'si', b'Sinhala'), (b'sk', b'Slovak'), (b'sl', b'Slovenian'), (b'sm', b'Samoan'), (b'sn', b'Shona'), (b'so', b'Somali'), (b'sq', b'Albanian'), (b'sr', b'Serbian'), (b'ss', b'Swati'), (b'st', b'Southern Sotho'), (b'su', b'Sudanese'), (b'sv', b'Swedish'), (b'sw', b'Swahili'), (b'ta', b'Tamil'), (b'te', b'Telugu'), (b'tg', b'Tajik'), (b'th', b'Thai'), (b'ti', b'Tigrinya'), (b'tk', b'Turkmen'), (b'tl', b'Tagalog'), (b'tn', b'Tswana'), (b'to', b'Tonga'), (b'tr', b'Turkish'), (b'ts', b'Tsonga'), (b'tt', b'Tatar'), (b'tw', b'Twi'), (b'ug', b'Uyghur'), (b'uk', b'Ukrainian'), (b'ur', b'Urdu'), (b'uz', b'Uzbek'), (b'vi', b'Vietnamese'), (b'vo', b'Volapuk'), (b'wo', b'Wolof'), (b'xh', b'Xhosa'), (b'yi', b'Yiddish'), (b'yo', b'Yoruba'), (b'za', b'Zhuang'), (b'zh', b'Chinese'), (b'zu', b'Zulu'), (b'nb_NO', b'Norwegian Bokmal'), (b'pt_BR', b'Brazilian Portuguese'), (b'uk_UA', b'Ukrainian'), (b'zh_CN', b'Simplified Chinese'), (b'zh_TW', b'Traditional Chinese')])),
('language', models.CharField(default=b'en', help_text="The language the project documentation is rendered in. Note: this affects your project's URL.", max_length=20, verbose_name='Language', choices=[(b'aa', b'Afar'), (b'ab', b'Abkhaz'), (b'af', b'Afrikaans'), (b'am', b'Amharic'), (b'ar', b'Arabic'), (b'as', b'Assamese'), (b'ay', b'Aymara'), (b'az', b'Azerbaijani'), (b'ba', b'Bashkir'), (b'be', b'Belarusian'), (b'bg', b'Bulgarian'), (b'bh', b'Bihari'), (b'bi', b'Bislama'), (b'bn', b'Bengali'), (b'bo', b'Tibetan'), (b'br', b'Breton'), (b'ca', b'Catalan'), (b'co', b'Corsican'), (b'cs', b'Czech'), (b'cy', b'Welsh'), (b'da', b'Danish'), (b'de', b'German'), (b'dz', b'Dzongkha'), (b'el', b'Greek'), (b'en', b'English'), (b'eo', b'Esperanto'), (b'es', b'Spanish'), (b'et', b'Estonian'), (b'eu', b'Basque'), (b'fa', b'Iranian'), (b'fi', b'Finnish'), (b'fj', b'Fijian'), (b'fo', b'Faroese'), (b'fr', b'French'), (b'fy', b'Western Frisian'), (b'ga', b'Irish'), (b'gd', b'Scottish Gaelic'), (b'gl', b'Galician'), (b'gn', b'Guarani'), (b'gu', b'Gujarati'), (b'ha', b'Hausa'), (b'hi', b'Hindi'), (b'he', b'Hebrew'), (b'hr', b'Croatian'), (b'hu', b'Hungarian'), (b'hy', b'Armenian'), (b'ia', b'Interlingua'), (b'id', b'Indonesian'), (b'ie', b'Interlingue'), (b'ik', b'Inupiaq'), (b'is', b'Icelandic'), (b'it', b'Italian'), (b'iu', b'Inuktitut'), (b'ja', b'Japanese'), (b'jv', b'Javanese'), (b'ka', b'Georgian'), (b'kk', b'Kazakh'), (b'kl', b'Kalaallisut'), (b'km', b'Khmer'), (b'kn', b'Kannada'), (b'ko', b'Korean'), (b'ks', b'Kashmiri'), (b'ku', b'Kurdish'), (b'ky', b'Kyrgyz'), (b'la', b'Latin'), (b'ln', b'Lingala'), (b'lo', b'Lao'), (b'lt', b'Lithuanian'), (b'lv', b'Latvian'), (b'mg', b'Malagasy'), (b'mi', b'Maori'), (b'mk', b'Macedonian'), (b'ml', b'Malayalam'), (b'mn', b'Mongolian'), (b'mr', b'Marathi'), (b'ms', b'Malay'), (b'mt', b'Maltese'), (b'my', b'Burmese'), (b'na', b'Nauru'), (b'ne', b'Nepali'), (b'nl', b'Dutch'), (b'no', b'Norwegian'), (b'oc', b'Occitan'), (b'om', b'Oromo'), (b'or', b'Oriya'), (b'pa', b'Panjabi'), (b'pl', b'Polish'), (b'ps', b'Pashto'), (b'pt', b'Portuguese'), (b'qu', b'Quechua'), (b'rm', b'Romansh'), (b'rn', b'Kirundi'), (b'ro', b'Romanian'), (b'ru', b'Russian'), (b'rw', b'Kinyarwanda'), (b'sa', b'Sanskrit'), (b'sd', b'Sindhi'), (b'sg', b'Sango'), (b'si', b'Sinhala'), (b'sk', b'Slovak'), (b'sl', b'Slovenian'), (b'sm', b'Samoan'), (b'sn', b'Shona'), (b'so', b'Somali'), (b'sq', b'Albanian'), (b'sr', b'Serbian'), (b'ss', b'Swati'), (b'st', b'Southern Sotho'), (b'su', b'Sudanese'), (b'sv', b'Swedish'), (b'sw', b'Swahili'), (b'ta', b'Tamil'), (b'te', b'Telugu'), (b'tg', b'Tajik'), (b'th', b'Thai'), (b'ti', b'Tigrinya'), (b'tk', b'Turkmen'), (b'tl', b'Tagalog'), (b'tn', b'Tswana'), (b'to', b'Tonga'), (b'tr', b'Turkish'), (b'ts', b'Tsonga'), (b'tt', b'Tatar'), (b'tw', b'Twi'), (b'ug', b'Uyghur'), (b'uk', b'Ukrainian'), (b'ur', b'Urdu'), (b'uz', b'Uzbek'), (b'vi', b'Vietnamese'), (b'vo', b'Volapuk'), (b'wo', b'Wolof'), (b'xh', b'Xhosa'), (b'yi', b'Yiddish'), (b'yo', b'Yoruba'), (b'za', b'Zhuang'), (b'zh', b'Chinese'), (b'zu', b'Zulu'), (b'nb_NO', b'Norwegian Bokmal'), (b'pt_BR', b'Brazilian Portuguese'), (b'es_MX', b'Mexican Spanish'), (b'uk_UA', b'Ukrainian'), (b'zh_CN', b'Simplified Chinese'), (b'zh_TW', b'Traditional Chinese')])),
Copy link
Contributor

@agjohnson agjohnson Feb 9, 2018

This shouldn't edit the migration, as it's already been executed in production. A new migration should be created.

Copy link
Contributor Author

@benjaoming benjaoming Feb 9, 2018

Django is currently very simple-minded: If you change ANY property of a field, it will require a migration.

It's okay to edit choices from the POV that you don't want a new migration every time that someone added another choice. It has no effect on the DB.

However, if you think it's a bad precedence, I can also add a new migration. IMO this doesn't scale with all the language codes that could potentially be added as project languages. You would just get a huge mess of migrations.

Another option would be to redefine the migration to find its choices from constants such that it would require no maintenance!

Copy link
Contributor

@agjohnson agjohnson Feb 9, 2018

Yeah, a new migration is the most technically correct, so I think we prefer that.

Though changing choices doesn't result in a database schema change, there will be an inconsistency between the model and the migration representation. Editing an existing migration worries me because of this and I don't know enough about django internals to say that there is no fallout from this.

Importing constants in a migration has the same effect, and is particularly worrisome because the code will evolve, whereas each migration is a point in time snapshot. Relying on code for defaults/etc in migrations results in migrations that aren't necessarily reproducible.

Copy link
Contributor Author

@benjaoming benjaoming Feb 9, 2018

@agjohnson

Yeah, a new migration is the most technically correct, so I think we prefer that.

Using a term like "technically correct" does not quite convince me, because I think you're incorrect about your assumption on migrations being sets of totally static code ;) So there's no real technical correctness to be established.

Relying on code for defaults/etc in migrations results in migrations that aren't necessarily reproducible.

Why would a list of tuples for choices result in something that's unpredictable when it's fetched from constants? Are those not constants? I can only see the extra no-op migration boiler code being a cause of much more concern if I wanted to maintain a code base?

I've had to update migrations before because of internal changes in Django and apps that shipped field types. Data migrations can also attract hind-sight updates with similar argumentation. Oh and then there's stuff like verbose_name where I'd also argue that adding a migration for every change is overkill.

The docs about field choices:

Note that choices can be any iterable object – not necessarily a list or tuple. This lets you construct choices dynamically.

Further:

choices is meant for static data that doesn’t change much, if ever.

I would think that both these comments indirectly support making a constant list of tuples play the part of a dynamic variable in the migration.

If you insist, I'll add a migration! But I won't without having warned against it and recommended another approach firstly...

As said before, the motivation for discussing this should be that many more languages are potentially subject to be added. The list from django.conf.global_settings.LANGUAGES isn't covered yet.

Copy link
Contributor Author

@benjaoming benjaoming Feb 9, 2018

@stsewd
Copy link
Member

@stsewd stsewd commented Mar 15, 2018

Hi @benjaoming can you make the changes requested about the migrations? I think that is the only thing needed to get this merged :)

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jun 6, 2018

I tested this locally, and did some research and it seems like editing the initial migration is fine in this case. 👍

@ericholscher ericholscher merged commit 17c5b2f into readthedocs:master Jun 6, 2018
1 check passed
@benjaoming
Copy link
Contributor Author

@benjaoming benjaoming commented Jun 8, 2018

Thanks for the verification @ericholscher -- I think this will mean a lot to the efforts required to get additional languages introduced in a simple, incremental fashion!

@stsewd sorry about not responding, that wasn't intentional, I have missed your comment. I'm glad that this was merged without further changes, though :) A dilemma could have been to have moved choices to a separate module containing constants data and imported it both into the models and migration modules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants