Skip to content

Remove redundant code paths #93

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

MrMino
Copy link
Collaborator

@MrMino MrMino commented Nov 8, 2022

We are now constrained on pip >= 21.2.4 (bcfd93c). This elliminates code paths that were there previously in order to cater to the older versions of Pip:

  • PipSession is now only found in pip._internal._network.session.
  • install_req_from_line is always used.
  • search_packages_info now always gives us a generator that yields pip._internal.commands.show._PackageInfo objects (not dict).

@MrMino MrMino requested a review from adamtheturtle November 8, 2022 01:57
@MrMino MrMino force-pushed the obsolete-imports branch 3 times, most recently from 19da09b to 9f6a653 Compare November 8, 2022 02:39
@MrMino
Copy link
Collaborator Author

MrMino commented Nov 8, 2022

Removing the extraneous monkeypatching revealed an interesting failure. I believe this is not related to this change, but still results in a bug. Requires further investigation.

@adamtheturtle
Copy link
Owner

Nice work.

I'd be happy if you want to split this up into "currently working" (we can merge that now) and "requires investigation".

You may have seen that I removed some patching in #88. I think that overall we can remove a lot of it and have a much more useful test suite.

@MrMino
Copy link
Collaborator Author

MrMino commented Nov 8, 2022

Will do.

MrMino added a commit to MrMino/pip-check-reqs that referenced this pull request Nov 9, 2022
We are now constrained on `pip >= 21.2.4` (bcfd93c). This commit
elliminates code paths that were there previously in order to cater to
the older versions of Pip.

- `search_packages_info` now always gives us a generator that yields
  `pip._internal.commands.show._PackageInfo` objects (not `dict`).

See 2c12ce5 for the rest of the changes
@MrMino
Copy link
Collaborator Author

MrMino commented Nov 12, 2022

Working subset has been merged within #95. The issue with this part boils down to how we treat paths. By converting all mocked paths to the absolute ones, the tests start passing, but there's still some missing coverage.

I think we should stop using relative paths completely and start using pathlib.Path everywhere.

@adamtheturtle
Copy link
Owner

Thanks for doing this work.

I think we should stop using relative paths completely and start using pathlib.Path everywhere.

Sounds fine to me.

By converting all mocked paths to the absolute ones, the tests start passing, but there's still some missing coverage.

Are you familiar with how to track down what that missing coverage is? If not, let me know and I'm happy to point you In the right direction.

@MrMino
Copy link
Collaborator Author

MrMino commented Nov 12, 2022

Are you familiar with how to track down what that missing coverage is? If not, let me know and I'm happy to point you In the right direction.

I am, it's just that I'm not sure if it is the step in the right direction. I think it could be sidestepped by some of the points from #97. There's a lot of code that could be simplified and made more testable by refactoring some of the code base.

I'll try some of that next week.

@adamtheturtle
Copy link
Owner

Thank you @MrMino !

Based on your remaining work here, and the prompts in #97, I made the tests modified here "integration tests" (no more monkeypatching!) in #114 , and then removed the unused code paths as you have done here in #115. I'll therefore close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants