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

add support for Pipfile.lock #216

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

add support for Pipfile.lock #216

wants to merge 1 commit into from

Conversation

nim65s
Copy link

@nim65s nim65s commented Apr 7, 2019

Hi,

This PR adds support for Pipfile.lock (https://github.com/pypa/pipfile), with a new -p option for the CLI, a really simple read_pipfile util inspired from the existing read_requirements, and a unit test.

Another option to add this suppport (and support other formats), would be to refactor read_requirements to let dparse get the requirements from multiple formats, maybe with a CLI switch with dparse.filetypes as choices.

@codecov
Copy link

codecov bot commented Apr 7, 2019

Codecov Report

Merging #216 into master will decrease coverage by 0.12%.
The diff coverage is 62.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #216      +/-   ##
==========================================
- Coverage   74.28%   74.15%   -0.13%     
==========================================
  Files           7        7              
  Lines         350      356       +6     
==========================================
+ Hits          260      264       +4     
- Misses         90       92       +2
Impacted Files Coverage Δ
safety/util.py 62.9% <100%> (+1.88%) ⬆️
safety/cli.py 54.87% <40%> (-0.82%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b81f90...dc696c4. Read the comment docs.

@GhostofGoes
Copy link
Contributor

Why have a separate argument for Pipfile instead of using -f and checking the filename?

@nim65s
Copy link
Author

nim65s commented Sep 6, 2019

Because we don't have access to the filename when using stdin.

But what we could do would be to check if the file contains valid JSON data, and in this case assume it is a Pipfile.lock. eg: nim65s@575636a

I'll let the maintainers of this project choose :)

Copy link
Contributor

@rafaelpivato rafaelpivato left a comment

Choose a reason for hiding this comment

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

The code looks really good. What I am trying to judge is whether it would make sense to add another file read function here. You pointed it very well at the beginning of your PR that we might instead need a refactor and properly use dparse library to read requirements from virtually any file.

I see why you added another option, but as @GhostofGoes noticed, we should just use -f instead. Detecting the file type, or being able to parse different file types, is for sure a dparse responsibility. What I would not like to do is to add one another option for each file type. At most, if we see dparse will not be able to detect the file type, we should use one option to point out different possible file types.

@alvarolmedo
Copy link

+1

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

Successfully merging this pull request may close these issues.

None yet

4 participants