Use the correct base repo for downloading PR sources

[popzxc]

Currently hintman attempts to download files from PR diff using prInfoUwner as a base repo, which doesn't work properly if the PR base is another repo (fork).

PrInfo should be extended with a field, e.g. prBaseRepo, which will be used to create a URL for file downloading.

For details, see the discussion at #156.

[vrom911]

But hintman works on the PRs made from forks as well. I don't quite get the issue and proposed changes. I guess that the current behaviour is correct as Hintman should work at the repo it's installed in. Could you please clarify what behaviour is expected?

[popzxc]

Yeah, see:

When I started working on #156, I created a PR to hintman-target (kowainik/hintman-target#39).

This PR is (mostly) the same as the one I ended with (in my own fork -- popzxc/hintman-target#1).

But if I try to use the PR to kowainik/hintman-target for tests:

forkPr1 <- makePr (PrNumber 39) (Sha "no-newline-at-file-end")

...it fails:

Failures:

  test/Test/Hint/NoNewlineAtFileEnd.hs:17:26: 
  1) Hintman hints, Comments are spawned for files without newline at file end, Spawns comment for README.md
       predicate failed on: []

  To rerun use: --match "/Hintman hints/Comments are spawned for files without newline at file end/Spawns comment for README.md/"

  test/Test/Hint/NoNewlineAtFileEnd.hs:19:26: 
  2) Hintman hints, Comments are spawned for files without newline at file end, Spawns comment for BigExample.hs
       predicate failed on: []

However, for a fork it works just fine:

forkPr1 <- makePrFrom (Owner "popzxc") (PrNumber 1) (Sha "no-newline-at-file-end")

Finished in 11.0149 seconds
27 examples, 0 failures

My initial thought was that the reason for it is that createFileDownloadUrl uses an owner repo as a part of URL, in my case the sample generated URL for file will be: https://raw.githubusercontent.com/kowainik/hintman-target/no-newline-at-file-end/BigExample.hs (note kowainik in URL), but the right URL should be https://raw.githubusercontent.com/popzxc/hintman-target/no-newline-at-file-end/BigExample.hs

However, I just made a test PR from fork in hintman-target (kowainik/hintman-target#40), and it indeed works.

Thus, I'm not sure why tests fail for kowainik/hintman-target#39 but pass for popzxc/hintman-target#1.

I've just re-checked it (just in case), and the issue persists locally at the moment of writing of this comment.

[popzxc]

How to reproduce:

1. Clone https://github.com/popzxc/hintman and switch to the no-newline-at-file-end branch.
2. Replace line 45 to forkPr1 <- makePr (PrNumber 39) (Sha "no-newline-at-file-end") in test/Data.hs.
3. Run stack test.

The tests should fail (while expected not to do so).

[vrom911]

I'm still not sure that we should use fork's owner, as at least one reason is that the fork owners don't have to install Hintman and we as the repo owners in which we installed Hintman want it to work on all PRs (even from forks) which is what is happening at the moment as far as I can see. As I see even from the linked PR that Hintman did its work there, so I wouldn't change this behaviour.

What is for tests, it's even better to have them all for the repo PRs', instead of the fork ones (forks can be deleted etc., and we don't have much control on that). So if you'd like we could make you a collaborator in there, but only if you would like so, no pressure there 🙂

[popzxc]

Regarding the behaviour:
probably, I incorrectly described the problem; the actual topic is the reason why tests fail if I try to use kowainik/hintman-target#39, but pass if I use PR within my own fork. With my understanding, there should be no difference between PR made from fork, and PR made from branch of the same repo, but as I can see with two listed test target PRs, this isn't correct for some reason.

Regarding being the collaborator -- sure! I'm really excited about hintman project and surely will be happy to work on it more 🙂

[chshersh]

According to the discussion, it looks like the problem with forks only when implementing tests. As @vrom911 noticed, we don't have control over forks, and it's better to keep all tests as branches to hintman-target. I don't think that it worth complicating the logic for tests and URL handling to support tests from forks. I'm entirely okay with creating PRs by myself or adding people as collaborators to hintman-target if they want to help the project, so this doesn't seem like an issue anymore 🙂