Skip to content

Allow RadioField #154

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

Closed
Garito opened this issue Jun 7, 2015 · 10 comments
Closed

Allow RadioField #154

Garito opened this issue Jun 7, 2015 · 10 comments

Comments

@Garito
Copy link

Garito commented Jun 7, 2015

Hi!
I would like to use a StringField with choices as, instead of a SelectField or MultiSelectField, RadioField
For that I modify the orm.py adding 2 lines of code
Instead of:

if field.choices:
            kwargs['choices'] = field.choices

            if ftype in self.converters:
                kwargs["coerce"] = self.coerce(ftype)
            if kwargs.pop('multiple', False):
                return f.SelectMultipleField(**kwargs)
            return f.SelectField(**kwargs)

I have

if field.choices:
            kwargs['choices'] = field.choices

            if ftype in self.converters:
                kwargs["coerce"] = self.coerce(ftype)
            if kwargs.pop('multiple', False):
                return f.SelectMultipleField(**kwargs)
            if kwargs.pop('radio', False):
                return f.RadioField(**kwargs)
            return f.SelectField(**kwargs)

Notice this 2 new lines:

            if kwargs.pop('radio', False):
                return f.RadioField(**kwargs)

I would prefer to make a pull request but I wasn't able to run the test (if you could point me to the test help I would apreciate it)

In the other hand, I would prefer that choices will be treated in the converter to allow subclassing better but this change will be ok for me

What do you think?

Thanks!

@Garito
Copy link
Author

Garito commented Jun 13, 2015

Hello?

@rozza
Copy link
Contributor

rozza commented Jun 15, 2015

Sounds fine to me @itsnauman @noirbizarre any thoughts?

@Garito
Copy link
Author

Garito commented Jun 15, 2015

Well, to be honest I don't since it breaks the converter's pattern because I would prefer to treat choices in the converter not in the preconverter like now

But this solve my specifc problem right now

@noirbizarre
Copy link
Collaborator

Concept seems fine to me but without a pull-request wa can't see the whole picture.

To run the tests: python setup.py nosetests

To run the tests on all Python versions and with different MongoEngine versions: tox

I will submit a PR with the testing documention and an updated CONTRIBUTING.rst.
(By the way, we should alos make tests pass on master: https://travis-ci.org/MongoEngine/flask-mongoengine/)

@Garito
Copy link
Author

Garito commented Jun 15, 2015

@noirbizarre sorry but your library is a mess right now because of the lack of update
If you run python setup.py nosetests you will end up with a mongo driver version 3 and your test will all fail
To make them pass you will need to uninstall it and install version 2.8 with the typical pip install pymongo==2.8
Besides that I'm trying to create a test case for my two modifications (I also added support for booleanfields)
Let me do this and push the pull request

@noirbizarre
Copy link
Collaborator

@Garito It's not my library, I'm a contributor trying to improve it. :)

The problem with PyMongo 3.0+ is a known upstream MongoEngine problem, already fixed and soon to be released: MongoEngine/mongoengine#946

With the PyMongo 2.8 workaround, is it working (and sufficient for you to perform your tests) ?

@Garito
Copy link
Author

Garito commented Jun 15, 2015

Yes, yes... thank you so much
Let me check if I can write the tests and I push the PR soon

@Garito
Copy link
Author

Garito commented Jun 15, 2015

I think I mix another change here: master...Garito:master
Is there a way to make the pull request without the TTL stuff?

@losintikfos
Copy link
Member

@Garito do you need this opened?

@Garito
Copy link
Author

Garito commented Jan 10, 2016

Everything seems to work as expected since 0.7.5
Thanks!

@Garito Garito closed this as completed Jan 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants