Opened 4 months ago

Closed 4 months ago

Last modified 4 months ago

#32986 closed Cleanup/optimization (fixed)

Possible dead or incorrect code in Lexer.create_token() related to TRANSLATOR_COMMENT_MARK

Reported by: Chris Jerdonek Owned by: Chris Jerdonek
Component: Template system Version: dev
Severity: Normal Keywords:
Cc: Claude Paroz Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

While working on 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, 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 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.

Change History (8)

comment:1 Changed 4 months ago by Mariusz Felisiak

Cc: Claude Paroz added
Triage Stage: UnreviewedAccepted

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.

comment:2 Changed 4 months ago 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 )

comment:3 in reply to:  2 Changed 4 months ago by Mariusz Felisiak

Replying to 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().

comment:4 Changed 4 months ago by Chris Jerdonek

Okay, sounds good.

comment:5 Changed 4 months ago by Chris Jerdonek

Has patch: set
Owner: changed from nobody to Chris Jerdonek
Status: newassigned

comment:6 Changed 4 months ago by Mariusz Felisiak

Triage Stage: AcceptedReady for checkin

comment:7 Changed 4 months ago by Mariusz Felisiak <felisiak.mariusz@…>

Resolution: fixed
Status: assignedclosed

In e79ae5c:

Fixed #32986 -- Removed unneeded str.find() call in Lexer.create_token().

Unnecessary since 47ddd6a4082d55d8856b7e6beac553485dd627f7.

comment:8 Changed 4 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 4fe3774:

Refs #32986 -- Moved TRANSLATOR_COMMENT_MARK to django.utils.translation.template.

Note: See TracTickets for help on using tickets.
Back to Top