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

make __str__ compatible with py2 #232

Merged
merged 3 commits into from Apr 19, 2018

Conversation

Projects
None yet
3 participants
@bloodywing
Contributor

bloodywing commented Jul 13, 2017

See a similar implementation in six:
https://github.com/circuits/circuits/blob/master/circuits/six.py#L840

Do we want to use the decorator and add __unicode__?

Fixes #229

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Jul 13, 2017

Codecov Report

❗️ No coverage uploaded for pull request base (master@f21e80a). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #232   +/-   ##
=========================================
  Coverage          ?   77.16%           
=========================================
  Files             ?       80           
  Lines             ?     6718           
  Branches          ?        0           
=========================================
  Hits              ?     5184           
  Misses            ?     1534           
  Partials          ?        0
Impacted Files Coverage Δ
circuits/core/values.py 88% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f21e80a...90039bb. Read the comment docs.

codecov-io commented Jul 13, 2017

Codecov Report

❗️ No coverage uploaded for pull request base (master@f21e80a). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #232   +/-   ##
=========================================
  Coverage          ?   77.16%           
=========================================
  Files             ?       80           
  Lines             ?     6718           
  Branches          ?        0           
=========================================
  Hits              ?     5184           
  Misses            ?     1534           
  Partials          ?        0
Impacted Files Coverage Δ
circuits/core/values.py 88% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f21e80a...90039bb. Read the comment docs.

@prologic

This comment has been minimized.

Show comment
Hide comment
@prologic

prologic Jul 15, 2017

Member

@spaceone What do you think?

Member

prologic commented Jul 15, 2017

@spaceone What do you think?

@bloodywing

This comment has been minimized.

Show comment
Hide comment
@bloodywing

bloodywing Jul 17, 2017

Contributor

@prologic after looking into six a bit more I realized that the best approach would be the decorator @python_2_unicode_compatible. The same method is used in Django: https://github.com/django/django/blob/1.10/django/db/models/manager.py#L12 for a long time now, which also makes this more familiar to people.
And it's cleaner to maintain, if circuits should ever ditch PY2 support one day.

Contributor

bloodywing commented Jul 17, 2017

@prologic after looking into six a bit more I realized that the best approach would be the decorator @python_2_unicode_compatible. The same method is used in Django: https://github.com/django/django/blob/1.10/django/db/models/manager.py#L12 for a long time now, which also makes this more familiar to people.
And it's cleaner to maintain, if circuits should ever ditch PY2 support one day.

bloodywing added a commit to bloodywing/circuits that referenced this pull request Jul 17, 2017

@prologic

This comment has been minimized.

Show comment
Hide comment
@prologic

prologic Jul 17, 2017

Member
Member

prologic commented Jul 17, 2017

spaceone added a commit that referenced this pull request Jul 17, 2017

make __str__ compatible with py2 (six-decorator) (#233)
* make __str__ compatible with py2 (six-decorator)
See also: #232 and #229
@prologic

This comment has been minimized.

Show comment
Hide comment
@prologic

prologic Apr 19, 2018

Member

Can you fix the conflicts?

Member

prologic commented Apr 19, 2018

Can you fix the conflicts?

@prologic

This comment has been minimized.

Show comment
Hide comment
@prologic
Member

prologic commented Apr 19, 2018

@prologic prologic merged commit f0cb656 into circuits:master Apr 19, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@bloodywing

This comment has been minimized.

Show comment
Hide comment
@bloodywing

bloodywing Apr 20, 2018

Contributor

@prologic Was this more of an issue with your travis config?

Contributor

bloodywing commented Apr 20, 2018

@prologic Was this more of an issue with your travis config?

@prologic

This comment has been minimized.

Show comment
Hide comment
@prologic

prologic Apr 20, 2018

Member
Member

prologic commented Apr 20, 2018

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