Printer bug: empty string literal args are dropped #1722
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
Followup investigation: the existing behavior isn't strictly incorrect, since a missing attribute or argument value is treated as the empty string. There's not an obvious best thing to do here in all cases.
To make both those cases do the expected thing, we'd need to make the printer treat attributes and arguments differently. Maybe that's OK? It seems like the least bad near-term option. Ultimately we would prefer to have a printer designed from the ground up for format preservation, but that would also require changes all the way down through the glimmer parser and handlebars parser, because information gets lost in both those layers. |
|
I'm very confused about how I can have a seemingly-unrelated CI failure here that isn't reproducing on main. |
ef4
force-pushed
the
printer-empty-string-literal
branch
from
92bb6ab to
948a249
Compare
NullVoxPopuli
approved these changes
Mar 4, 2025
git-online-helper bot
added a commit
to patricklx/glimmer-vm
that referenced
this pull request
May 13, 2025
Update lockfile x ope Merge pull request glimmerjs#1726 from glimmerjs/put-prettier-testing-in-the-smoke-tests-project Move prettier tests to the smoke-tests project Update release-plan Don't forget repo meta update Printer bug: empty string literal args are dropped proposed fix lint:fix Merge pull request glimmerjs#1727 from glimmerjs/update-release-plan Update release-plan Merge pull request glimmerjs#1722 from glimmerjs/printer-empty-string-literal Printer bug: empty string literal args are dropped Set node-version to 22 Merge pull request glimmerjs#1728 from glimmerjs/set-node-version-to-22 Set node-version to 22 in plan-release/publish Prepare Release using 'release-plan' Merge pull request glimmerjs#1715 from glimmerjs/release-preview Prepare Release Revert "Prepare Release" Merge pull request glimmerjs#1729 from glimmerjs/revert-1715-release-preview Revert "Prepare Release" Go back to using configs from before glimmerjs#1727 Merge pull request glimmerjs#1730 from glimmerjs/fix-release-plan fix release-plan Prepare Release using 'release-plan' Merge pull request glimmerjs#1731 from glimmerjs/release-preview Prepare Release Fix bench post-install ignore-workspace -> ignore-workspace-root-check Force CI=false for bench Update bench-packages.mts Use postinstall script to opt out of postinstall +x bah Merge pull request glimmerjs#1732 from glimmerjs/fix-post-install Fix bench post-install Restore {{debugger}} behavior Merge pull request glimmerjs#1734 from glimmerjs/restore-debugger Restore {{debugger}} behavior Prepare Release using 'release-plan' Manually update the release-preview to force @glimmer/runtime to release Add pkgJSONPath Add constraints array (empty) Add entry to constraints arary Add impact Merge pull request glimmerjs#1733 from glimmerjs/release-preview Prepare Release Force release Merge pull request glimmerjs#1735 from glimmerjs/force-release Force release pnpm repo:update:metadata Merge pull request glimmerjs#1736 from glimmerjs/nvp/update-meta pnpm repo:update:metadata Removing editor.rulers This drives me batty every time I open this repo. I think it's fair to say it falls into opinionated IDE settings that can stay in a personal config instead of the repo. Merge pull request glimmerjs#1738 from glimmerjs/editor-rulers Removing editor.rulers Re-add package.json#types for tsconfig's moduleResolution=node10 Merge pull request glimmerjs#1741 from glimmerjs/add-back-types-for-moduleResolution=node10 Re-add support for tsconfig's moduleResolution=node10 Prepare Release using 'release-plan' Merge pull request glimmerjs#1737 from glimmerjs/release-preview Prepare Release Upgrade handlebars parser Upgrade to nppm 10.6.5 Merge pull request glimmerjs#1743 from glimmerjs/upgrade-handlebars-parser Upgrade handlebars parser Merge pull request glimmerjs#1744 from glimmerjs/nvp/update-pnpm Upgrade to pnpm 10.6.5 test
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The printer (in all modes, not just the hacked-on "codemod mode") loses empty string literal arguments: