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

feat(PyProject::detect): detect unconventional requirements.txt (#1323) #1336

Merged
merged 5 commits into from
Apr 19, 2024

Conversation

crsche
Copy link
Contributor

@crsche crsche commented Apr 18, 2024

Proposed Changes

Detect a wider set of Python requirements files using regex (^requirements\S*\.txt), since pip explicitly does not require naming the file requirements.txt.

Closes #1323

Release Notes

Change Python requirements file matching from requirements.txt to requirements*.txt

Copy link
Contributor

@mkenigs mkenigs left a comment

Choose a reason for hiding this comment

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

Thanks, overall looks good! I left a bunch of nonblocking comments and nits, so you can decide whether or not to address those.

The other thing is we could consider installing every requirements*.txt file like @zmitchell mentioned here #1323 (comment). Up to you if you want to make that change

cli/flox/src/commands/init/python.rs Outdated Show resolved Hide resolved
cli/flox/src/commands/init/python.rs Outdated Show resolved Hide resolved
cli/flox/src/commands/init/python.rs Show resolved Hide resolved
cli/flox/src/commands/init/python.rs Outdated Show resolved Hide resolved
cli/flox/src/commands/init/python.rs Outdated Show resolved Hide resolved
cli/flox/src/commands/init/python.rs Outdated Show resolved Hide resolved
…nd other misc. refactoring

refactoring based on comments in flox#1336

Signed-off-by: Conor Scheidt <conorrs@protonmail.com>
Copy link
Contributor

@mkenigs mkenigs left a comment

Choose a reason for hiding this comment

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

Changes look good! Sorry I missed this before but describe_reason needs to be updated as well. Right now it's printing

Flox detected a Python project with the following Python provider(s):

* latest python (requirements.txt)

And describe_customization also hardcodes requirements.txt:

Installs the dependencies from the requirements.txt to the venv.

@mkenigs
Copy link
Contributor

mkenigs commented Apr 18, 2024

Also could you run cargo clippy and address any warnings? (there are a few warnings unrelated to your change that I fixed if you want to rebase against main so you don't get the noise)

@crsche
Copy link
Contributor Author

crsche commented Apr 18, 2024

Will do, sorry for the hassle, I'm a bit new to contribution.

@mkenigs
Copy link
Contributor

mkenigs commented Apr 18, 2024

Will do, sorry for the hassle, I'm a bit new to contribution.

No worries we need to add a CI job that runs clippy and add it to the contributor guidelines, that's on us!

@crsche
Copy link
Contributor Author

crsche commented Apr 18, 2024

Should we update the describe_reason method in the Provider trait to allow for a non-static Cow<str>? This would be necessary to list all the requirements files, something like:

Flox detected a Python project with the following Python provider(s):

* latest python (requirements.txt, requirements_dev.txt, ...)

Same idea with describe_customization.

@mkenigs
Copy link
Contributor

mkenigs commented Apr 18, 2024

Should we update the describe_reason method in the Provider trait to allow for a non-static Cow<str>? This would be necessary to list all the requirements files, something like:

Yeah I think that's necessary for the behavior we want

Signed-off-by: Conor Scheidt <me@crsche.com>
Copy link
Contributor

@mkenigs mkenigs left a comment

Choose a reason for hiding this comment

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

Thanks!

@mkenigs mkenigs enabled auto-merge April 19, 2024 16:55
@mkenigs mkenigs added this pull request to the merge queue Apr 19, 2024
Merged via the queue into flox:main with commit d2fe08f Apr 19, 2024
29 checks passed
@crsche crsche deleted the init-detect-custom-requirements-txt branch April 19, 2024 17:33
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.

flox init finds alternate naming of requirements.txt
2 participants