[Django] #32986: Possible dead or incorrect code in Lexer.create_token() related to TRANSLATOR_COMMENT_MARK
Django
TRANSLATOR_COMMENT_MARK
------------------------------------------------+------------------------
Reporter: Chris Jerdonek | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Template system | Version: dev
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
While working on [https://github.com/django/django/pull/14739 this PR] for
ticket #32919, I noticed that `Lexer.create_token()` has a branch of code
that looks like it might be wrong or not needed.
Specifically,
[https://github.com/django/django/blob/4f3acf957918843b4c40ff2edfb929bcfaa3730e/django/template/base.py#L391
this line] seems always to be true. It's true both when
`TRANSLATOR_COMMENT_MARK` is found and not found. It's only false when
`token_string` starts with `TRANSLATOR_COMMENT_MARK` since `str.find()` is
used (as it returns the index). However, when I tried modifying the code
in [https://github.com/django/django/pull/14742 this test PR] to raise an
exception in the false code path, the test suite still passed. So it's not
clear to me if the `str.find()` call is needed, or if it's just incorrect.
--
Ticket URL: <https://code.djangoproject.com/ticket/32986>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
TRANSLATOR_COMMENT_MARK
Reporter: Chris Jerdonek | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Template system | Version: dev
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* cc: Claude Paroz (added)
* stage: Unreviewed => Accepted
Comment:
`content` for non-translator comments is unnecessary, so I'd change this
to `token_string.find(TRANSLATOR_COMMENT_MARK) > -1`.
Incorrect since its introduction in
17b329ae08f9e3886da2baafc9e53949000480f9.
--
Ticket URL: <https://code.djangoproject.com/ticket/32986#comment:1>
Django
TRANSLATOR_COMMENT_MARK
Reporter: Chris Jerdonek | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Template system | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Chris Jerdonek):
In that case, you'd want `TRANSLATOR_COMMENT_MARK in token_string`, right?
(See note at https://docs.python.org/3/library/stdtypes.html#str.find )
--
Ticket URL: <https://code.djangoproject.com/ticket/32986#comment:2>
Django
TRANSLATOR_COMMENT_MARK
Reporter: Chris Jerdonek | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Template system | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak):
Replying to [comment:2 Chris Jerdonek]:
> In that case, you'd want `TRANSLATOR_COMMENT_MARK in token_string`,
right? (See note at
https://docs.python.org/3/library/stdtypes.html#str.find )
TBH, checking `TRANSLATOR_COMMENT_MARK` is probably completely unnecessary
here since 47ddd6a4082d55d8856b7e6beac553485dd627f7. We can remove
`find()`.
--
Ticket URL: <https://code.djangoproject.com/ticket/32986#comment:3>
Django
TRANSLATOR_COMMENT_MARK
Reporter: Chris Jerdonek | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Template system | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Chris Jerdonek):
Okay, sounds good.
--
Ticket URL: <https://code.djangoproject.com/ticket/32986#comment:4>
Django
TRANSLATOR_COMMENT_MARK
Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Component: Template system | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Chris Jerdonek):
* owner: nobody => Chris Jerdonek
* status: new => assigned
* has_patch: 0 => 1
Comment:
PR: https://github.com/django/django/pull/14743
--
Ticket URL: <https://code.djangoproject.com/ticket/32986#comment:5>
Django
TRANSLATOR_COMMENT_MARK
Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/32986#comment:6>
Django
TRANSLATOR_COMMENT_MARK
Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek
Component: Template system | Version: dev
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"e79ae5c317c4fe94c9ed9593309c07d9cf3bbbdf" e79ae5c]:
{{{
#!CommitTicketReference repository=""
revision="e79ae5c317c4fe94c9ed9593309c07d9cf3bbbdf"
Fixed #32986 -- Removed unneeded str.find() call in Lexer.create_token().
Unnecessary since 47ddd6a4082d55d8856b7e6beac553485dd627f7.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32986#comment:7>
Django
TRANSLATOR_COMMENT_MARK
Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek
Cleanup/optimization | Status: closed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"4fe3774c729f3fd5105b3001fe69a70bdca95ac3" 4fe3774]:
{{{
#!CommitTicketReference repository=""
revision="4fe3774c729f3fd5105b3001fe69a70bdca95ac3"
Refs #32986 -- Moved TRANSLATOR_COMMENT_MARK to
django.utils.translation.template.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32986#comment:8>