Skip to content

Don't display quotes around strings in --dump_stamp_tool#292

Merged
Colecf merged 1 commit into
google:masterfrom
Colecf:fix_stamp_dump_format
Aug 5, 2025
Merged

Don't display quotes around strings in --dump_stamp_tool#292
Colecf merged 1 commit into
google:masterfrom
Colecf:fix_stamp_dump_format

Conversation

@Colecf
Copy link
Copy Markdown
Collaborator

@Colecf Colecf commented Aug 4, 2025

To match ckati.

@Colecf Colecf requested a review from danw August 4, 2025 23:36
@Colecf Colecf requested a review from a team as a code owner August 4, 2025 23:36
Copy link
Copy Markdown
Collaborator

@danw danw left a comment

Choose a reason for hiding this comment

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

Is the purpose here so that you can diff between ckati and rkati? In general the quoting is probably a good idea to make any oddities more obvious.

Comment thread src-rs/regen_dump.rs
@Colecf
Copy link
Copy Markdown
Collaborator Author

Colecf commented Aug 5, 2025

Is the purpose here so that you can diff between ckati and rkati? In general the quoting is probably a good idea to make any oddities more obvious.

No, it's because this dump tool is used by other tools that expect to parse it in a specific format. I think this is the only real user, and they only use --files, but I changed the rest for consistency.

@danw
Copy link
Copy Markdown
Collaborator

danw commented Aug 5, 2025

Even ignoring the string encoding issue in the inner comment, this interface really isn't great for parsing like that -- it's entirely valid for filenames to include newline characters, though I really hope nobody does that. (not to mention that the makefile syntax doesn't handle whitespace in filenames either)

That file will also need to be updated to ignore rkati the way it ignores ckati.

@Colecf Colecf merged commit bb7850a into google:master Aug 5, 2025
2 checks passed
@Colecf Colecf deleted the fix_stamp_dump_format branch August 5, 2025 16:12
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