murex icon indicating copy to clipboard operation
murex copied to clipboard

Backslashes and code comments

Open piegamesde opened this issue 2 years ago • 7 comments

exec echo "one" \
        "two" \
        "three"\
        "four"\ # test
        "five" \ # test
        "six"

prints one two three four five six. I didn't check in detail, but those additional spaces are very likely some additional command line arguments that should not be there. This is causing interference with the application's argument parser.

Edit: Oh wow, that's sneaky: apparently some of those spaces are zero-width and only visible with monospace fonts?

image

piegamesde avatar Nov 10 '21 22:11 piegamesde

Ack, I've clearly cocked something up in the parser there and somehow also failed to add tests around that particular condition. I'll look into this and hopefully have a fix in the next release.

Apologies for the inconvenience there.

lmorg avatar Nov 11 '21 09:11 lmorg

Btw I found a workaround: simply put the backslash behind the comment to make things work again:

exec echo "one" \
        "two" \
        "three" \
        "four" # test \
        "five" # test \
        "six"

Not sure if that is intended though

piegamesde avatar Nov 11 '21 11:11 piegamesde

It wasn't and you've potentially unearthed another bug there.

I'll give the parser some love over the next few weeks. If truth be told, it's some of the oldest code in the project and I had intended to rewrite it at some point anyway because it's just a simple single pass compiler and thus doesn't handle nested subshells correctly either. Which was fine in the early POC stages of the shell but we're a long way beyond that now.

I'll probably do this as a two stage release though:

  • first stage is to do some TDD and put a quick hack in to get those tests working.
  • Then I'll start on the rewrite as I should have some confidence that any rewrite wouldn't introduce new regression bugs

Thanks as always for raising these issues.

lmorg avatar Nov 12 '21 07:11 lmorg

Written a patch to resolve the slash before a comment problem (pushed to develop). It's a very experimental patch and certainly doesn't address the full parser rewrite. But it's hopefully enough to close off this issue, once it hits master.

The interesting thing is, during my testing I noticed that neither Bash nor Zsh actually support either scenario. But I think we can do better. So now both of the following tests pass in murex:

{
	Block: `out: one\
			two \
			three\ # test
			four \ # test
			five
		out: six`,
	Stdout: "one two three four five\nsix\n",
},
{
	Block: `out: one\
			two \
			three # test \
			four # test  \
			five
		out: six`,
	Stdout: "one two three four five\nsix\n",
},

lmorg avatar Nov 22 '21 00:11 lmorg

Thank you.

during my testing I noticed that neither Bash nor Zsh actually support either scenario. But I think we can do better

Yes. There is a StackOverflow question on that topic (can't find it right now), the accepted answer is a really ugly hack.

So now both of the following tests pass in murex

Hm, does this mean that I can add the backslash either before the comment symbol or before the end of line, and both will work?

piegamesde avatar Nov 22 '21 01:11 piegamesde

Hm, does this mean that I can add the backslash either before the comment symbol or before the end of line, and both will work?

In theory, yes.

My thinking is that, as a developer, formatting the code is an activity that doesn't generate value aside readability (ie it's not part of the problem solving, it's just tidying up the aesthetics). So in supporting both placements of the escape character murex is removing any cognitive overhead nor research needed for performing that activity thus allowing the developer to move quickly back onto the task that does add value (ie the logic of the code rather than the formatting of it).

The reason I say "in theory" is because I need to write a few more tests to ensure there aren't any weird parsing glitches created by this patch plus the interactive terminal may behave slightly different too since it is parsed line by line. It was late then I pushed this commit through :) I hope to get more thorough tests in place over the next few days.

lmorg avatar Nov 22 '21 07:11 lmorg

Current patch has been into master. Leaving this issue open for now as I still want to write more tests plus backport the change to the interactive terminal. However v2.4 was already looking like a large release so didn't want to hold that up any longer. I might sneak a few updates to this issue in point releases rather than waiting for a v2.5 release.

lmorg avatar Dec 08 '21 01:12 lmorg