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

__str__ is broken for MSFList #76

Open
devxpy opened this issue Jan 14, 2018 · 9 comments
Open

__str__ is broken for MSFList #76

devxpy opened this issue Jan 14, 2018 · 9 comments

Comments

@devxpy
Copy link

devxpy commented Jan 14, 2018

If the choices are defined like this,

    CHOICE1 = '1'
    CHOICE2 = '2'
    CHOICE3 = '3'
    CHOICE4 = '4'
    ANSWER_CHOICES = ((CHOICE1, 'A'),
                      (CHOICE2, 'B'),
                      (CHOICE3, 'C'),
                      (CHOICE4, 'D'))

then the str method returns None, None, None ..

it can be easily fixed by replacing
[msgl.choices.get(int(i)) if i.isdigit() else msgl.choices.get(i) for i in msgl]

with

[msgl.choices.get(i) for i in msgl.choices if i in self.msgl]

@blag
Copy link
Collaborator

blag commented Jan 15, 2018

I can't fix this because it would break what other people expect __str__ to serialize.

Why can't you use integers instead of strings? Like so:

    CHOICE1 = 1
    CHOICE2 = 2
    CHOICE3 = 3
    CHOICE4 = 4
    ANSWER_CHOICES = ((CHOICE1, 'A'),
                      (CHOICE2, 'B'),
                      (CHOICE3, 'C'),
                      (CHOICE4, 'D'))

Keep in mind that this project is a hack on top of Django's MultipleChoiceField, and will have problems and limitations due to that.

If you cannot use integers, I would recommend using Django's normal many to many relation for this. Sorry I don't have better advice.

@devxpy
Copy link
Author

devxpy commented Jan 15, 2018

I tried doing that, but I have existing data and it wont convert the types when i migrate, for the existing data.
Anyhow, i resorted to using a custom method on my model that does the job :)

@blag
Copy link
Collaborator

blag commented Jan 29, 2018

I tried doing that, but I have existing data and it wont convert the types when i migrate, for the existing data

Yep, this a major downside to using this project - it becomes difficult if not impossible to upgrade the database around it. 🙁

I'm glad you found a workaround though. Cheers!

@devxpy
Copy link
Author

devxpy commented Feb 16, 2018

I remember now why i did '1' instead of just 1.
Because it won't let me save manually from a script with integers as the input.

Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/vagrant/quiz/helper_scripts.py", line 188, in populate_db
    owner=user,
  File "/home/vagrant/.pyenv/versions/venv/lib/python3.6/site-packages/django/db/models/manager.py", line 82, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/home/vagrant/.pyenv/versions/venv/lib/python3.6/site-packages/django/db/models/query.py", line 417, in create
    obj.save(force_insert=True, using=self.db)
  File "/vagrant/quiz/Exams/models.py", line 70, in save
    super().save(*args, **kwargs)
  File "/home/vagrant/.pyenv/versions/venv/lib/python3.6/site-packages/django/db/models/base.py", line 729, in save
    force_update=force_update, update_fields=update_fields)
  File "/home/vagrant/.pyenv/versions/venv/lib/python3.6/site-packages/django/db/models/base.py", line 759, in save_base
    updated = self._save_table(raw, cls, force_insert, force_update, using, update_fields)
  File "/home/vagrant/.pyenv/versions/venv/lib/python3.6/site-packages/django/db/models/base.py", line 842, in _save_table
    result = self._do_insert(cls._base_manager, using, fields, update_pk, raw)
  File "/home/vagrant/.pyenv/versions/venv/lib/python3.6/site-packages/django/db/models/base.py", line 880, in _do_insert
    using=using, raw=raw)
  File "/home/vagrant/.pyenv/versions/venv/lib/python3.6/site-packages/django/db/models/manager.py", line 82, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/home/vagrant/.pyenv/versions/venv/lib/python3.6/site-packages/django/db/models/query.py", line 1125, in _insert
    return query.get_compiler(using=using).execute_sql(return_id)
  File "/home/vagrant/.pyenv/versions/venv/lib/python3.6/site-packages/django/db/models/sql/compiler.py", line 1280, in execute_sql
    for sql, params in self.as_sql():
  File "/home/vagrant/.pyenv/versions/venv/lib/python3.6/site-packages/django/db/models/sql/compiler.py", line 1233, in as_sql
    for obj in self.query.objs
  File "/home/vagrant/.pyenv/versions/venv/lib/python3.6/site-packages/django/db/models/sql/compiler.py", line 1233, in <listcomp>
    for obj in self.query.objs
  File "/home/vagrant/.pyenv/versions/venv/lib/python3.6/site-packages/django/db/models/sql/compiler.py", line 1232, in <listcomp>
    [self.prepare_value(field, self.pre_save_val(field, obj)) for field in fields]
  File "/home/vagrant/.pyenv/versions/venv/lib/python3.6/site-packages/django/db/models/sql/compiler.py", line 1172, in prepare_value
    value = field.get_db_prep_save(value, connection=self.connection)
  File "/home/vagrant/.pyenv/versions/venv/lib/python3.6/site-packages/django/db/models/fields/__init__.py", line 767, in get_db_prep_save
    return self.get_db_prep_value(value, connection=connection, prepared=False)
  File "/home/vagrant/.pyenv/versions/venv/lib/python3.6/site-packages/multiselectfield/db/fields.py", line 140, in get_db_prep_value
    value = self.get_prep_value(value)
  File "/home/vagrant/.pyenv/versions/venv/lib/python3.6/site-packages/multiselectfield/db/fields.py", line 136, in get_prep_value
    return '' if value is None else ",".join(value)
TypeError: sequence item 0: expected str instance, int found

@blag blag reopened this Feb 17, 2018
@blag
Copy link
Collaborator

blag commented Feb 17, 2018

Reopened because this is something I might be able to fix.

I can simply convert all keys to strings, would that work for you?

Try modifying the file in the Vagrant container - change line 136 of /home/vagrant/.pyenv/versions/venv/lib/python3.6/site-packages/multiselectfield/db/fields.py to this:

        return '' if value is None else ",".join(value)

and see if that works. If so I'll just have it convert all keys to strings.

@audiolion
Copy link

audiolion commented Feb 20, 2018

I am working on moving a Django project on py2 to py3 and I got an error related to this, I had some sql migrations produced from changing python versions and removing from __future__ import unicode_literals

in lib/python3.6/site-packages/multiselectfield/db/fields.py", line 136
    return '' if value is None else ",".join(value)
TypeError: sequence item 0: expected str instance, int found

it looks like the code you suggested is already in there.

The sql migration code produced:

migrations.AlterField(
            model_name='event',
            name='employee_type',
            field=multiselectfield.db.fields.MultiSelectField(choices=[('i', 'Interpreters'), ('c', 'RTC-NT'), ('n', 'Other')], default='i', max_length=4),
        ),

@devxpy
Copy link
Author

devxpy commented Sep 10, 2018

@blag It would have been better if this used some standard serialization format like JSON, or at least python's __repr__(), so that it differentiates between an int and str objects.

But that would break everything.

Why not test if the CHOICES have an int or str, and do type conversion based on that?

Also, why use .get() as opposed to __getitiem__(), which returns an ambiguous None instead of an exception?

@mohammedhammoud
Copy link

+1

@erikjyrmann
Copy link

I'm not sure if this was mentioned here, but in case it helps anyone, this breaks printing out Django model fields, like:

object = Object.objects.get(id=id)
print(object.field)

the internal representation, however, is available:
repr(object.field)

So if the issue is not resolved, the string values of MSFList can be accessed with repr()

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

No branches or pull requests

5 participants