search

thoth-station / micropipenv

A lightweight wrapper for pip to support requirements.txt, Pipenv and Poetry lock files or converting them to pip-tools compatible output. Designed for containerized Python applications but not limited to them.
https://pypi.org/project/micropipenv/
GNU Lesser General Public License v3.0
231 stars 25 forks source link

Return full pipfile_entry dict instead of cherry-picking values #257

Closed VannTen closed 1 year ago

VannTen commented 1 year ago

Related Issues and Dependencies

fix #256 …

This introduces a breaking change

This Pull Request implements

If there is an unknown key, we won't use it in the rest of the code. This way, we gain free support for dependencies using git file, etc.

Caveat

I'm not completely sure of the full impact of the fix. But I think we only use specific keys in the returned dictionnary. Hence we should not have any problem with unknown ones, because we just won't use them. But that also means we might accept invalid Pifile.

frenzymadness commented 1 year ago

I think this approach is fine. Could you please add some tests for it? You can at least transfer the bug report into a regression test so we'll have one more use case covered in tests.

VannTen commented 1 year ago

I plan to, I should do that this week.

VannTen commented 1 year ago

Looks like some pip version related warning, I suppose pip 23 is new. I'll take a look, if time allows.

frenzymadness commented 1 year ago

Just change

https://github.com/thoth-station/micropipenv/blob/fe8c6fd6fc29b80d154dbf1d34f5266a477e9b89/micropipenv.py#L51

to <=23.0.

VannTen commented 1 year ago

/approve I'll rebase on top of #258 once it's in.

VannTen commented 1 year ago

I'll do that, at least I'll know how the release part works.

VannTen commented 1 year ago

@frenzymadness can you put a /lgtm ?

sesheta commented 1 year ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: frenzymadness, VannTen

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/thoth-station/micropipenv/blob/master/OWNERS)~~ [VannTen,frenzymadness] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
frenzymadness commented 1 year ago

/lgtm

frenzymadness commented 1 year ago

I already approved it last week and I see no significant difference after the lgtm command.

frenzymadness commented 1 year ago

So, before the lgtm command was posted by me, I saw that I could merge this PR. Now, after the lgtm command, I cannot do that unless it passes all the checks or I use my admin privileges to bypass the rules for the protected branch. And the sesheta cheks will never finish I guess. I'm inclined to remove the bot again.

VannTen commented 1 year ago

looks like I don't have push right to master. Btw, is the release to pypi handled by github as well ?

frenzymadness commented 1 year ago

looks like I don't have push right to master. Btw, is the release to pypi handled by github as well ?

Yes, but it requires a tag which is not possible to do via PR so you can open a PR with a new version, changelog etc. and I'll merge it, tag it and push it to master which should lead to a new release on PyPI.

VannTen commented 1 year ago

And I had a local and signed tag :/. Oh well.

frenzymadness commented 1 year ago

So, do you want to be able to push directly to master?

VannTen commented 1 year ago

To do releases, yeah.

We could have some automation for releases and changelog, but I don't have the time to invest in that right now, so for the time being let's keep it manual.