Skip to content

Consolidate placeholder tracking in the source tracker. - #150

Merged
davepeck merged 3 commits into
t-strings:mainfrom
ianjosephwilson:ian/prep_for_custom_errors_part2
Aug 9, 2026
Merged

Consolidate placeholder tracking in the source tracker.#150
davepeck merged 3 commits into
t-strings:mainfrom
ianjosephwilson:ian/prep_for_custom_errors_part2

Conversation

@ianjosephwilson

Copy link
Copy Markdown
Contributor
  • Morph SourceTracker into an Iterator.
  • Move placeholder injection (and tracking) internally into the SourceTracker.
  • Redirect TemplateParser's placeholder extraction to proxy methods of the `SourceTracker. The parser still needs to know when its just looking at a snapshot versus when it is "marking" the placeholder as found. Not ideal but this composition feels better and more distinct from the regular parsing duties.
  • Use configure_source_tracker factory function because later we bind the PlaceholderConfig to members other than PlaceholderState that depend on the size of the placeholders (defined by the config).

The SourceTracker methods get_expression and format_starttag will be removed in another PR and those responsibilities will be in the SourceReader (constructed and accessed during exceptions).

@ianjosephwilson

Copy link
Copy Markdown
Contributor Author

@davepeck Thanks for merging those changes. Here is the next "chunk" that continues to move things into alignment with the error messaging.

@davepeck davepeck left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! Two very small and ignorable comments:

  1. There are a couple places in parser.py where we do if self.source: or if self.source and ...:. Maybe these should become if self.source is not None for future safety (it's fine now, but what happens if we add, say, __len__?)

  2. Should we have a variant of test_iter that tests t"" and t"one-string-only" cases?

As for find_placeholders and remove_placeholders: I think it works, but agree you have to be cautious.

Anyway, extremely minor feedback! LMK when you're good for a merge.

@davepeck
davepeck self-requested a review August 8, 2026 22:21
@ianjosephwilson

Copy link
Copy Markdown
Contributor Author

@davepeck I added a parametrized test for those simple string cases you mentioned. If this looks ok I think you should merge it so I can start prep'ing the next part.

@davepeck
davepeck merged commit 9826409 into t-strings:main Aug 9, 2026
1 check passed
@davepeck

davepeck commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

@ianjosephwilson Merged; thanks!

By the way, one thing I've been contemplating is moving the source structure to something more standard in the python world, specifically:

src/tdom/... <-- top level src/ which seems to be the thing to do these days
tests/... <-- top level tests/ that mirror the structure of src/tdom/ aka src/tdom/sub/whippersnapper.py gets tests/sub/test_whippersnapper.py

This is just a bunch of churn, so I want to avoid it until you are in a position to take it. If that's now, cool; if that's, like, after the error handling stuff is settled, cool too -- just let me know. (And if you don't like this structure, LMK what you'd prefer!)

@ianjosephwilson

Copy link
Copy Markdown
Contributor Author

@ianjosephwilson Merged; thanks!

By the way, one thing I've been contemplating is moving the source structure to something more standard in the python world, specifically:

src/tdom/... <-- top level src/ which seems to be the thing to do these days tests/... <-- top level tests/ that mirror the structure of src/tdom/ aka src/tdom/sub/whippersnapper.py gets tests/sub/test_whippersnapper.py

This is just a bunch of churn, so I want to avoid it until you are in a position to take it. If that's now, cool; if that's, like, after the error handling stuff is settled, cool too -- just let me know. (And if you don't like this structure, LMK what you'd prefer!)

Haha don't you dare DAVE! Thanks for asking and I think these are both good ideas for once we get the error stuff in assuming nothing else is going on. But it would probably be safer to wait in case I need to scissor-and-glue-stick together more sub-sub PRs for us to digest to get this thing in.

@davepeck

davepeck commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

But it would probably be safer to wait

Yup, no rush on this one. Will do later.

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.

2 participants