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

Trying to fix issue #9 #10

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@ghost

ghost commented Dec 27, 2015

Hi Richard,

This is my attempt to fix the issue reported by me almost a month ago, the problem was the __len__ method overloading. I know the reason to have that method overloaded, unfortunately that is causing tastypie don't work with tagulous, also by inspecting the source code, I cannot find one occurrence where the len method is used. Perhaps, if we need a method to count the tags, we should use: TheModel.objects.count()

Hoping this change doesn't cause any conflict.

Martin Guerrero added some commits Dec 10, 2015

Martin Guerrero
comment __len__ operator overloading
this allows us to associate tags tastypie resources
@radiac

This comment has been minimized.

Show comment
Hide comment
@radiac

radiac Dec 27, 2015

Owner

Great, thanks for the patch! Yes, I think the idea for len was in the theme of ease of use - but even so, thinking about it, in this context len should probably be giving the number of chars in the string. If it's causing problems for other things, there's nothing lost by dropping it - people can always call len(...get_tag_list()).

I've started working on tagulous again this week; will merge this when I get a chance after Christmas.

Owner

radiac commented Dec 27, 2015

Great, thanks for the patch! Yes, I think the idea for len was in the theme of ease of use - but even so, thinking about it, in this context len should probably be giving the number of chars in the string. If it's causing problems for other things, there's nothing lost by dropping it - people can always call len(...get_tag_list()).

I've started working on tagulous again this week; will merge this when I get a chance after Christmas.

Martin Guerrero
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jun 4, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 5cb63cf on slackmart:master into 898dc44 on radiac:master.

coveralls commented Jun 4, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 5cb63cf on slackmart:master into 898dc44 on radiac:master.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jun 4, 2016

Hi @radiac

In order to make the tests pass, I've changed the test_len method.

.. any change to get this merged?

ghost commented Jun 4, 2016

Hi @radiac

In order to make the tests pass, I've changed the test_len method.

.. any change to get this merged?

radiac added a commit that referenced this pull request Feb 26, 2017

@ghost ghost closed this Jun 26, 2017

radiac added a commit that referenced this pull request Jul 2, 2017

@radiac

This comment has been minimized.

Show comment
Hide comment
@radiac

radiac Jul 2, 2017

Owner

I'm really sorry for the absurd delay dealing with this. After deprecating __len__ in 0.12, this is now merged into my working branch and will be in 0.13 when it's eventually released.

Thanks once again for the patch!

Owner

radiac commented Jul 2, 2017

I'm really sorry for the absurd delay dealing with this. After deprecating __len__ in 0.12, this is now merged into my working branch and will be in 0.13 when it's eventually released.

Thanks once again for the patch!

This issue was closed.

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