Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

expand: fix sequential '\' in double quote literals #1107

Merged

Conversation

cfogrady
Copy link
Contributor

@cfogrady cfogrady commented Nov 4, 2024

Fix for #1106

@mvdan
Copy link
Owner

mvdan commented Nov 4, 2024

Thanks! I actually avoid unit tests where possible, and instead test the exposed API. In this case I would add integration tests in the interpreter, for example adding to these around line 400 in interp_test.go:

	// escaped chars
	{"echo a\\b", "ab\n"},
	{"echo a\\ b", "a b\n"},
	{"echo \\$a", "$a\n"},
	{"echo \"a\\b\"", "a\\b\n"},
	{"echo 'a\\b'", "a\\b\n"},
	{"echo \"a\\\nb\"", "ab\n"},
	{"echo 'a\\\nb'", "a\\\nb\n"},
	{`echo "\""`, "\"\n"},
	{`echo \\`, "\\\n"},
	{`echo \\\\`, "\\\\\n"},
	{`echo \`, "\n"},

I know that means there isn't test coverage in the same package, but I think proper coverage across the module is still fine. And, ultimately, most of these bugs in the expand package only matter for users of the interp package.

@mvdan
Copy link
Owner

mvdan commented Nov 4, 2024

And avoiding unit tests also means that you don't need to add one more function to test.

@cfogrady
Copy link
Contributor Author

cfogrady commented Nov 4, 2024

And avoiding unit tests also means that you don't need to add one more function to test.

For clarity, do you prefer to keep the logic inlined? My default is to split out what I would consider to be tangential logic because I think it helps readability, but you're the maintainer.

@mvdan
Copy link
Owner

mvdan commented Nov 4, 2024

Yes, please keep the code where it was - the func isn't particularly large or very indented, so I don't think it requires splitting up just yet.

@cfogrady
Copy link
Contributor Author

cfogrady commented Nov 5, 2024

Function is inlined once again, and the (in my opinion) essential test cases have been added to the interp_test.go test cases.

Copy link
Owner

@mvdan mvdan left a comment

Choose a reason for hiding this comment

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

Thank you! The change is subtle but I do see how my previous logic was wrong with repeated backslashes, as rather than treating them like escape pairs, I would keep on skipping over all of them.

@mvdan
Copy link
Owner

mvdan commented Nov 8, 2024

There was a minor gofmt issue which I fixed. I will squash-merge, as I prefer to keep the git history tidy. For future PRs, feel free to squash into a single commit yourself with a good commit message, and I'll merge as-is.

@cfogrady
Copy link
Contributor Author

cfogrady commented Nov 8, 2024

Sounds good!

@mvdan
Copy link
Owner

mvdan commented Nov 8, 2024

(as usual got sidetracked waiting for CI to pass, merging now)

@mvdan mvdan merged commit 5919e5a into mvdan:master Nov 8, 2024
8 checks passed
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