Fix extra incorrectly shown on installation error

[sepehr-rs]

Fixes #13618
This resolves the issue, though I’m not certain it’s the ideal place for the fix. If needed, we can move the solution further down in the stack.

[notatallshaw]

I've not spent any time looking at the code, but I would be pretty nervous about removing the marker information until right before the error message.

Even if that marker information isn't used now someone might want to use it later and be surprised to find all marker information stripped, and there's no comment as to why that happened.

[sepehr-rs]

I've not spent any time looking at the code, but I would be pretty nervous about removing the marker information until right before the error message.

Even if that marker information isn't used now someone might want to use it later and be surprised to find all marker information stripped, and there's no comment as to why that happened.

Thanks for the review, @notatallshaw!
I moved the change further down so the marker info is preserved internally and only stripped at error-message time.

[notatallshaw]

Update the test test_install_special_extra in tests/functional/test_install_extras.py to include something like this at the end:

    assert "No matching distribution found for missing_pkg" in result.stderr, str(result)
    assert "extra ==" not in result.stderr, str(result)

Then I think you're going to find two situations you're not handeling right now:

1. When the requirement has a parent, you need to move your string manipulation to the top of _report_single_requirement_conflict so it also gets applied to the req_disp variable for logging
2. The logging and raising of raise DistributionNotFound in PackagingFinder.find_requirement in src/pip/_internal/index/package_finder.py

[notatallshaw]

I'm realizing I wrote the above comment in a bit of a rush and I've not verified the output is actually misleading in that test. So feel free to correct me if I made a mistake, I'll take a other look in the next few days when I have time to review again.

[notatallshaw]

I'm going to move this to 26.1 milestone, I'm generally happy with this approach, but it needs tests, either as new or modified existing ones, and I don't see a rush to get it out (this error has been confusing for a long time).

[sepehr-rs]

Hi @notatallshaw, thanks for the review. Sorry for the delay. I’ve updated the code and tests based on your feedback. Would appreciate another look when you have a moment.
Thanks again!

[danielhollas]

Not a pip developer, but hopefully some helpful review comments...

Also this needs more tests I think, right now it seems that only one of the two modified paths is tested?

[sepehr-rs]

Hi @danielhollas, thanks a lot for the helpful review! I've addressed your comments, but I’m not entirely clear on your note about the assertion test. Could you clarify what you meant?

[danielhollas]

@sepehr-rs thanks. You can disregard my test assertion comment, but I think there's still an issue with how you override req_disp variable.

[danielhollas]

I think this is still wrong as it overrides the req_disp set above.

Suggested change

	req_disp = re.sub(r';\s*extra\s*==\s*".*?"', "", str(req)).strip()
	req_str = req_disp
	req_disp = re.sub(r';\s*extra\s*==\s*".*?"', "", req_disp).strip()

[danielhollas]

Instead of defining req_str perhaps it makes sense to just use req_disp here? It's already used above for logging.

[notatallshaw]

I took a quick look at this danielhollas's comments all look correct to me.

[sepehr-rs]

Hi, sorry for the delay. Things have been hectic over here in Iran and I only recently got access to GitHub again. I’ve committed the changes proposed by @danielhollas. Please let me know what you think!

[notatallshaw]

Sorry, I think this PR still needs some more review and work, but I'm very unlikely to have time to review before 26.1.

[danielhollas]

Things have been hectic over here in Iran and I only recently got access to GitHub again.

Oof, what an understatement, I am very sorry. :-(

Thank you for the changes, I think that there is at least one thing that needs fixing, otherwise looks reasonable to me, but I am not a pip maintainer, just trying to help with a review :-)