Skip to content
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

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

Merged
merged 2 commits into from
Feb 16, 2023

Conversation

VannTen
Copy link
Member

@VannTen VannTen commented Jan 27, 2023

Related Issues and Dependencies

fix #256

This introduces a breaking change

  • Yes
  • No

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.

@sesheta sesheta requested a review from KPostOffice January 27, 2023 11:04
@sesheta sesheta added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 27, 2023
@frenzymadness
Copy link
Collaborator

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
Copy link
Member Author

VannTen commented Jan 30, 2023 via email

@VannTen
Copy link
Member Author

VannTen commented Feb 7, 2023

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

@frenzymadness
Copy link
Collaborator

Just change

_SUPPORTED_PIP_STR = ">=9,<=22.3.1" # Respects requirement in setup.py and latest pip to release date.

to <=23.0.

@VannTen
Copy link
Member Author

VannTen commented Feb 8, 2023

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

@sesheta sesheta added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 8, 2023
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.
@VannTen VannTen force-pushed the fix/pipfile_entry_no_version branch from 9911a30 to cf60063 Compare February 9, 2023 08:41
Copy link
Collaborator

@frenzymadness frenzymadness left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it and I've verified that the test failed with the previous version. Do you want to do a new release? You can just add a changelog, bump the version and push it to master with a corresponding tag.

@VannTen
Copy link
Member Author

VannTen commented Feb 10, 2023 via email

@VannTen
Copy link
Member Author

VannTen commented Feb 16, 2023

@frenzymadness can you put a /lgtm ?

@sesheta
Copy link
Member

sesheta commented Feb 16, 2023

[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 [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
Copy link
Collaborator

/lgtm

@sesheta sesheta added the lgtm Indicates that a PR is ready to be merged. label Feb 16, 2023
@frenzymadness
Copy link
Collaborator

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

@frenzymadness
Copy link
Collaborator

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.

@sesheta sesheta merged commit 0a7cdfe into thoth-station:master Feb 16, 2023
@VannTen
Copy link
Member Author

VannTen commented Feb 17, 2023

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

@frenzymadness
Copy link
Collaborator

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
Copy link
Member Author

VannTen commented Feb 17, 2023 via email

@frenzymadness
Copy link
Collaborator

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

@VannTen
Copy link
Member Author

VannTen commented Feb 23, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Key error raised on requirements subcommand when VCS dependency is specified
3 participants