Compare difference between primer runs and post comment

[DanielNoord]

• Add yourself to CONTRIBUTORS if you are a new contributor.
• Write a good description on what the PR does.

Type of Changes

	Type
✓	🔨 Refactoring

Description

Ref. #5364

Example can be seen here:
DanielNoord#145

We can't show the code in a code snippet easily as the normal functionality Github offers only works for permalinks within the same repository. So we would need to create a script that downloads the code ourselves or store it as an artefact in the CI runs. For now, I think this should be fine though.

I'm not 100% confident that this won't crash as it is quite hard to test this. The comment workflow needs to be on the default branch and I could only do so much testing on my local fork.
Therefore I have only set three packages up for priming with this system so that the others can be taken care of by the others.

[coveralls]

Pull Request Test Coverage Report for Build 2402208248

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 95.51%

---

Totals
Change from base Build 2402203893:	0.0%
Covered Lines:	16230
Relevant Lines:	16993

---

💛 - Coveralls

[Pierre-Sassoulas]

This is awesome 🤩 !

[jacobtylerwalls]

Terrific! I have two comments that I think are important to discuss before we merge this and turn this feature "on", even though I should have brought them up earlier :D

[jacobtylerwalls]

I'm definitely in favor of punting the cyclic-import issues if they are hard to solve, but I'm wondering if we merge main especially after #6725 the problems just go away! A couple CI runs would be nice to see!

[DanielNoord]

I'm definitely in favor of punting the cyclic-import issues if they are hard to solve, but I'm wondering if we merge main especially after #6725 the problems just go away! A couple CI runs would be nice to see!

Well I jusst merged main. Let me remove and see if it works on my local branch.

[DanielNoord]

@jacobtylerwalls That doesn't look good:
DanielNoord#145

You said you could reproduce locally? Could you check what is up with those cyclic-imports? It seems like there are similar messages, but not similar enough.

[DanielNoord]

@jacobtylerwalls It just seems as if cylic-import is not deterministic enough. Not sure if it is worth the time to investigate for this PR.

[jacobtylerwalls]

I'm about to take a look. I agree we will punt if we're doing everything right. In the meantime, did you notice

git.exc.NoSuchPathError: /home/runner/work/pylint/pylint/tests/.pylint_primer_tests/pytest-dev/pytest

?

[DanielNoord]

I'm about to take a look. I agree we will punt if we're doing everything right. In the meantime, did you notice

git.exc.NoSuchPathError: /home/runner/work/pylint/pylint/tests/.pylint_primer_tests/pytest-dev/pytest

?

Yeah that's another thing which I am not sure we can/should fix right now.

Whenever we add a new package to prime on the main run won't have cached that package and thus the run will fail. We could obviously recreate the cache, but that doesn't really help: main won't have output for that package so we will fail other stages in the process where we assume that main and PR have the same packages in their output dictionary (output_main.txt and output_pr.txt).

Edit: It's stuff like this that makes me nudge towards only running with labels now. There is just a lot to consider and the difficulty of relying on main (something which mypy/pydocstringformatter don't do for example) makes it quite hard to test/fix stuff. Normally it is obviously not ideal to try and "fix the car while it is running", but in this instance it might actually be easier? 😅

[jacobtylerwalls]

Got it. Forgot that adding pytest was in this changeset. We're likely to do this anyway, but when we add more packages from now let's add em in PRs that only do just that.

[DanielNoord]

Got it. Forgot that adding pytest was in this changeset. We're likely to do this anyway, but when we add more packages from now let's add em in PRs that only do just that.

Completely agree. In fact, we might even consider not running Primer / Run whenever we change the to_prime_packages JSON file, primer_main.yaml and primer_comment.yaml. All of those changes will always need to be on main to work so it doesn't really make sense to run when any of those files gets changed.

[jacobtylerwalls]

If you don't mind, could you just do that now: open and merge a PR that adds pytest? And then this PR should get past that failure. It's worth going a little slower on this first PR, it helps me understand what I'm reviewing!

[jacobtylerwalls]

Do we know what the double slashes in the URLs are doing there?
.../blob/main//astroid/...

[jacobtylerwalls]

What about wrapping the diffed items in a <details>? That's always nice to keep the pull request discussion from getting overwhelming. Some functional changes could be quite overwhelming, if a new feature causes 250 messages in Django, 40 in pytest, 40 in music21, etc.

[DanielNoord]

What about wrapping the diffed items in a <details>? That's always nice to keep the pull request discussion from getting overwhelming. Some functional changes could be quite overwhelming, if a new feature causes 250 messages in Django, 40 in pytest, 40 in music21, etc.

Good idea! That might interfere with code snippets at some point, but for now it is absolutely better!

[jacobtylerwalls]

@jacobtylerwalls It just seems as if cylic-import is not deterministic enough. Not sure if it is worth the time to investigate for this PR.

Agreed. I see this locally even after the changes we just did. Let's go back to ignoring them and add a comment that it's currently nondeterministic. We have various issues open, I don't know that we need another.

[DanielNoord]

Do we know what the double slashes in the URLs are doing there?
.../blob/main//astroid/...

Should be fixed!

What about wrapping the diffed items in a <details>? That's always nice to keep the pull request discussion from getting overwhelming. Some functional changes could be quite overwhelming, if a new feature causes 250 messages in Django, 40 in pytest, 40 in music21, etc.

Added!

@jacobtylerwalls It just seems as if cylic-import is not deterministic enough. Not sure if it is worth the time to investigate for this PR.

Agreed. I see this locally even after the changes we just did. Let's go back to ignoring them and add a comment that it's currently nondeterministic. We have various issues open, I don't know that we need another.

Re-added!

Edit: Test run and comment can be seen here: DanielNoord#145 (comment)

[jacobtylerwalls]

Wonderful!

[DanielNoord]

@Pierre-Sassoulas Do you want to do one final review? We changes some stuff around based on @jacobtylerwalls's very useful suggestions!

[DudeNr33]

Just wanted to say that this is awesome and will be very valuable in the future! Great work. 👍