resolve symlinks before reading solution files

[lcowell]

Resolves symlinks before reading file contents in. This should be OS
independent as I'm using the filepath package to resolve the symlink.

Fixes #354

[Tonkpils]

I thought we were doing this already somewhere. However, from my understanding of #354, the issue was that the name of the file we were using to submit was the one of the original file rather than the one from the symlink. Does this correct that issue?

[lcowell]

Yeah, it looks like ioutil.Readfile may be resolving symlinks already.

I misunderstood the original issue. Let me take another look at this.

[lcowell]

@Insti @Tonkpils I've pushed up a test to verify the name. It looks like the CLI is already doing the right thing and is using the name of the symlink. This test passes with or without the code I added.

Can someone see what I've done wrong? Is this a non-issue?

[kytrinyx]

Can someone see what I've done wrong?

I can't.

Is this a non-issue?

Maybe?

@Insti would you weigh in here, since you reported the original issue? I think we may have a gap in our understanding of the problem.

[Tonkpils]

The original issue was this:

Original file name: foo_awesome.rb
Symlink name: foo_link.rb -> foo_awesome.rb

$ exercism submit foo_link.rb

The submitted file name is foo_awesome.rb instead of foo_link.rb.

What @Insti was referring to is that he expected the argument passed to submit to be the name of the file rather than the file linked to.

[Insti]

@Tonkpils has summarised the issue correctly.

[Insti]

I suspect the problem is here:

https://github.com/exercism/cli/blob/follow_symlinks/cmd/submit.go#L78-L85

The symlink is resolved, but the original name is not kept.

Given that the new TestFollowSymlink shows that NewIteration does what is expected and follows symlinks correctly, can we just delete lines 78-85 in submit.go and have it all work?

I don't currently have a Go development environment set up to check this myself.

[Tonkpils]

Given that the new TestFollowSymlink shows that NewIteration does what is expected and follows symlinks correctly, can we just delete lines 78-85 in submit.go and have it all work?

Ah, so we're EvalSymlink twice, once in submit.go and once in iteration.go. I think it'd be safe to do what you suggested, we can still maintain the symlink for the debug output purpose but send the original filename. We should also rename that variable file to filename to be more clear that it's the name of the file and not the contents of it.

[lcowell]

Thanks @Insti I didn't even think to check that the outer loop would be resolving the symlinks.

I've squashed the commits down to what really needed to happen. I didn't change any code in iteration.go, but I've left the test in there because it documents the desired behaviour. Let me know if you think that should be removed.

[Insti]

Great.

I'm usually in favour of more tests, but is the test actually testing anything that is not already tested? (Other than if ioutil.ReadFile(filename) is doing the right thing following symlinks? Which isn't our job to test.)

I'd rather see a test that detected the original bug(?) in submit.go prematurely resolving symlinks, which was then fixed by deleting the relevant lines.

(I really appreciate the work you're putting into this @lcowell ! ❤️)

Edit:

I'd rather see a test that detected the original bug(?) in submit.go

This may be way easier said than done, given that it is a huge method with no existing tests. 😢

[Insti]

This is ready to be merged.

[kytrinyx]

Thanks @Insti, for checking back on this. And thanks @lcowell for getting this fixed! Merging now.