Skip to content

PEP template incorrectly adds whitespace between code and following punctuation #825

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
gvanrossum opened this issue Sep 21, 2015 · 11 comments
Labels
app/peps Relates to the peps app

Comments

@gvanrossum
Copy link
Member

When a PEP uses backticks to render something as code and immediately follows it with punctuation (typically a comma or close paren) the template inserts a space -- apparently in the fixed-width font, which makes it even uglier. The legacy rendering doesn't have this problem.

Example: in https://www.python.org/dev/peps/pep-0495/ search for "is passed as tzinfo" finds two instances of this problem on one line. Compare to http://legacy.python.org/dev/peps/pep-0495/ where these two are rendered correctly.

@berkerpeksag berkerpeksag added the app/peps Relates to the peps app label Sep 22, 2015
@willingc
Copy link
Contributor

willingc commented Oct 2, 2015

Refactor these files to clean up rendering of peps.
peps/converters.py
peps/management/commands/generate_pep_pages.py
peps/list.html

@willingc
Copy link
Contributor

willingc commented Oct 2, 2015

@berkerpeksag Now that we've updated the pep generation doc, I have tried to run the generate_pep_pages.py. I am running into the AttributeError for HTMLParseError (since that was deprecated). I tried a few easy workarounds upgrading Django, upgrading BS4, upgrading to dev version of Sphinx but none of these gave an easy clean solution.

Going to 🌊. Will try to give another look after the weekend.

@berkerpeksag
Copy link
Member

Can you share the full traceback? I haven't had a chance to look into this yet.

@willingc
Copy link
Contributor

willingc commented Oct 2, 2015

This is using the manual install. I haven't yet tried with vagrant.

 ✘  ~p/pythondotorg   master  ./manage.py generate_pep_pages          ⦕my3dpydot⦖
Traceback (most recent call last):
  File "./manage.py", line 8, in <module>
    execute_from_command_line(sys.argv)
  File "/Users/carol/Projects/my3dpydot/lib/python3.5/site-packages/django/core/management/__init__.py", line 385, in execute_from_command_line
    utility.execute()
  File "/Users/carol/Projects/my3dpydot/lib/python3.5/site-packages/django/core/management/__init__.py", line 354, in execute
    django.setup()
  File "/Users/carol/Projects/my3dpydot/lib/python3.5/site-packages/django/__init__.py", line 18, in setup
    from django.utils.log import configure_logging
  File "/Users/carol/Projects/my3dpydot/lib/python3.5/site-packages/django/utils/log.py", line 13, in <module>
    from django.views.debug import ExceptionReporter, get_exception_reporter_filter
  File "/Users/carol/Projects/my3dpydot/lib/python3.5/site-packages/django/views/debug.py", line 10, in <module>
    from django.http import (HttpResponse, HttpResponseServerError,
  File "/Users/carol/Projects/my3dpydot/lib/python3.5/site-packages/django/http/__init__.py", line 4, in <module>
    from django.http.response import (
  File "/Users/carol/Projects/my3dpydot/lib/python3.5/site-packages/django/http/response.py", line 13, in <module>
    from django.core.serializers.json import DjangoJSONEncoder
  File "/Users/carol/Projects/my3dpydot/lib/python3.5/site-packages/django/core/serializers/__init__.py", line 23, in <module>
    from django.core.serializers.base import SerializerDoesNotExist
  File "/Users/carol/Projects/my3dpydot/lib/python3.5/site-packages/django/core/serializers/base.py", line 6, in <module>
    from django.db import models
  File "/Users/carol/Projects/my3dpydot/lib/python3.5/site-packages/django/db/models/__init__.py", line 6, in <module>
    from django.db.models.query import Q, QuerySet, Prefetch  # NOQA
  File "/Users/carol/Projects/my3dpydot/lib/python3.5/site-packages/django/db/models/query.py", line 13, in <module>
    from django.db.models.fields import AutoField, Empty
  File "/Users/carol/Projects/my3dpydot/lib/python3.5/site-packages/django/db/models/fields/__init__.py", line 18, in <module>
    from django import forms
  File "/Users/carol/Projects/my3dpydot/lib/python3.5/site-packages/django/forms/__init__.py", line 6, in <module>
    from django.forms.fields import *  # NOQA
  File "/Users/carol/Projects/my3dpydot/lib/python3.5/site-packages/django/forms/fields.py", line 18, in <module>
    from django.forms.utils import from_current_timezone, to_current_timezone
  File "/Users/carol/Projects/my3dpydot/lib/python3.5/site-packages/django/forms/utils.py", line 15, in <module>
    from django.utils.html import format_html, format_html_join, escape
  File "/Users/carol/Projects/my3dpydot/lib/python3.5/site-packages/django/utils/html.py", line 16, in <module>
    from .html_parser import HTMLParser, HTMLParseError
  File "/Users/carol/Projects/my3dpydot/lib/python3.5/site-packages/django/utils/html_parser.py", line 12, in <module>
    HTMLParseError = _html_parser.HTMLParseError
AttributeError: module 'html.parser' has no attribute 'HTMLParseError'
 ✘  ~p/pythondotorg   master 

@berkerpeksag
Copy link
Member

Ah, yes, we don't support Python 3.5 right now. Prod is on 3.3 and Vagrant box is on 3.4. We need to upgrade to Django 1.8(and I'm not sure that they have a workaround for this in Django 1.8) before upgrading to Python 3.5, but there are some blockers like #591 and tastypie. I have a local WIP branch for Django 1.8 upgrade.

@willingc
Copy link
Contributor

willingc commented Oct 3, 2015

Leaving this here as a reminder since I believe this is where it was fixed in Django django/django@b07aa52

I will give some thought on whether there's another alternative method for pep -> html that will be a quicker fix for the formatting and still keep the images up to date. We've done some conversions with the Jupyter notebooks to html.

@berkerpeksag
Copy link
Member

Hi @willingc, do you have some time to work on this or would you like me to take over? I'd be happy to review and merge a patch on this :) Thanks!

@willingc
Copy link
Contributor

Hi @berkerpeksag I would be happy for you to take over here. I'm just returning from a Japan vacation and have a bunch of work stuff to do over the next couple of weeks. Thanks!

@Mariatta
Copy link
Member

Hi @willingc and @berkerpeksag,
I'm interested to work on this issue. Seem's like you made some improvements already. Can I know what is still pending that need to be worked on?
Thanks.

@berkerpeksag
Copy link
Member

Hi @Mariatta, thanks for your interest working on this. I think I already fixed this, but forgot send a PR. I'm away from my computer at the moment. I will check when I get home and let you know what's needed (we probably need a test case.)

@berkerpeksag
Copy link
Member

berkerpeksag commented Sep 17, 2016

Ok, found my branch, rebased and pushed to master...berkerpeksag:825-pep-prettify.

What's missing here is that the lack of test coverage of convert_pep_page(). The following code block does most of the work, but unfortunately it doesn't really well tested: https://github.com/python/pythondotorg/blob/master/peps/converters.py#L123-L156

I remember I tested it manually and it worked fine, but it would be nice to have some more real test coverage :) You can see up-to-date information by running:

$ coverage run manage.py test peps
$ coverage report -m

If you'd like to work on this, we might need to design some sort of framework to easily test the converter code. Currently, we have a fake PEP repository located at https://github.com/python/pythondotorg/tree/master/peps/tests/fake_pep_repo. We might need to create PEP repos or PEPs dynamically to test every branch of the converter code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app/peps Relates to the peps app
Projects
None yet
Development

No branches or pull requests

4 participants