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

Set section length dynamically #6

Merged
merged 2 commits into from
Feb 8, 2018
Merged

Conversation

wanshot
Copy link
Contributor

@wanshot wanshot commented Jan 28, 2018

Hi tell-k
In the case of long model verbose, syntax error of reStructuredText will result.

I set the section length dynamically.

However, I used wcwidth for this fix, but if you do not want to rely on external libraries, reject it

@coveralls
Copy link

coveralls commented Jan 28, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 62b0251 on wanshot:dynamic_section into cc3009e on tell-k:master.

Copy link
Owner

@tell-k tell-k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wanshot

First of all thanks for your nice PR.

I have a question for you.
Why shold we need wcwdith?

I do not think that an exact character width is necessary. By converting to bytes, you can get a length longer than the exact character width. That's enough.

# py3
from django.utils.encoding import force_bytes
from wcwidth import wcswidth

print(len(force_bytes('日本語'))) # => 9 (This is enough length).
print(wcswidth('日本語'))         # => 6

In other words, it can be a simpler implementation.
Please try this implementaion and modfied your PR.

https://gist.github.com/tell-k/a6b7a2bfb15952fa898c9dcda95efba7

If this is not good, please report again.
Thx for your help :)

tox.ini Outdated
@@ -6,6 +6,7 @@ deps =
coverage
mock
pbr<1.4
wcwidth
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line isn't needed. Because wcwidth exists in install_requires of setup.py. you should write only the testing library here.

@wanshot
Copy link
Contributor Author

wanshot commented Jan 31, 2018

Thanks for the reply:satisfied:

Certainly precise number of characters was not necessary.

The suggested modification is better than what I thought. I will refer to it...
I fix it and PR again!

@tell-k
Copy link
Owner

tell-k commented Jan 31, 2018

I fix it and PR again!

yeah!
i'm look forward to your next PR.

thx.

@wanshot
Copy link
Contributor Author

wanshot commented Feb 6, 2018

I will be glad if you look fixed at your convenience!

@tell-k tell-k merged commit c813006 into tell-k:master Feb 8, 2018
@tell-k
Copy link
Owner

tell-k commented Feb 8, 2018

@wanshot Sorry too late my reply. Thank you for your help.

@tell-k
Copy link
Owner

tell-k commented Feb 8, 2018

I've just released v1.9.0 https://pypi.python.org/pypi/django-modelsdoc/0.1.9

@wanshot
Copy link
Contributor Author

wanshot commented Feb 8, 2018

@tell-k thank you!! 🎉

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

Successfully merging this pull request may close these issues.

3 participants