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 basic metadata importing to init command #265

Merged
merged 38 commits into from
Jun 21, 2023

Conversation

cnpryer
Copy link
Contributor

@cnpryer cnpryer commented May 30, 2023

Summary of changes

  • Add configparser for INI file parsing
  • Add monotrail-utils for RequirementsTxt requirements file parsing.
  • rye init by default will attempt an import from setup.cfg and setup.py files (setup.py is prioritized)
  • Add --no-import argument
  • Add -r/--requirements and --dev-requirements arguments for requirements files

How it works

If --no-import is not used:

  1. Attempt to read metadata from setup.py
  2. Attempt to read from setup.cfg
  3. Attempt to read requirements from -r/--requirements/--dev-requirements

Tag #191


TODO:

  • import setup.cfg first-pass
  • import setup.py first-pass
  • import requirements first-pass (see unsupported features)

@mitsuhiko
Copy link
Collaborator

One thought I have is that if --import is not passed, but there is something importable it should probably not do something stupid but let the user know that they can import instead.

@cnpryer
Copy link
Contributor Author

cnpryer commented May 31, 2023

One thought I have is that if --import is not passed, but there is something importable it should probably not do something stupid but let the user know that they can import instead.

e18f12d adds an assumed import. If you'd prefer waiting for user input or something similar/more explicit I'll modify it.

rye/src/cli/init.rs Outdated Show resolved Hide resolved
@cnpryer
Copy link
Contributor Author

cnpryer commented Jun 8, 2023

I'm changing this PR to only tag #191 instead of closing. I think we'd want to close it when/if requirements parsing is fully supported, when we're able to import from poetry-managed projects (or other tooling), and after we'd decided if we see a future where Rye fully supports setup file managed projects.

@cnpryer cnpryer changed the title WIP: Add metadata importing to init command Add metadata importing to init command Jun 12, 2023
@cnpryer
Copy link
Contributor Author

cnpryer commented Jun 12, 2023

Marking this as ready for a review, but I'd like to do more testing later. Here's where it's at so far:

See PR description for callouts.

  1. We use the monotrail-utils crate for requirements parsing.
  2. We perform the import when rye init is called and a setup.py or setup.cfg is found
  3. You cant import all supported requirements formats (see this link).

@cnpryer cnpryer marked this pull request as ready for review June 12, 2023 20:25
}
if let Some(Some(reqs)) = section.get("install_requires") {
reqs.lines()
.filter_map(|x| Requirement::from_str(x).ok())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will silently fail

reqs.iter()
.map(ToString::to_string)
.map(escape_string)
.filter_map(|x| Requirement::from_str(&x).ok())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will silently fail

rye/src/cli/init.rs Outdated Show resolved Hide resolved
Comment on lines +487 to +488
if let Some(Value::String(name)) = json.get("name") {
if metadata.name.is_none() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
if let Some(Value::String(name)) = json.get("name") {
if metadata.name.is_none() {
if metadata.name.is_none() {
if let Some(Value::String(name)) = json.get("name") {

Might flip these

@mitsuhiko
Copy link
Collaborator

This looks pretty good but I will need to do some tests still. Will try to get to it on the weekend.

@cnpryer
Copy link
Contributor Author

cnpryer commented Jun 15, 2023

Have you thought about how we want to do testing in general? I'm happy to introduce some here for this change, but I want to align with broader testing of rye.

@ischaojie
Copy link
Contributor

I've been concerned about the lack of extensive testing for rye, and if someone can add some tests as a reference (I lack experience in this area of rust), I'd be happy to contribute.

@mitsuhiko
Copy link
Collaborator

mitsuhiko commented Jun 16, 2023

Rye definitely has reached the point where it needs tests. I have started something a while back based on insta-cmd but it was all quite dependent on external sources and felt super fragile.

I made a meta issue for the testing here: #322

@cnpryer cnpryer changed the title Add metadata importing to init command Add basic metadata importing to init command Jun 16, 2023
@cnpryer cnpryer mentioned this pull request Jun 17, 2023
@mitsuhiko mitsuhiko merged commit 4a477d1 into astral-sh:main Jun 21, 2023
@mitsuhiko
Copy link
Collaborator

I will merge this now. I did not make progress on tests yet, but that should not hold this up.

mitsuhiko added a commit that referenced this pull request Jun 21, 2023
@cnpryer cnpryer deleted the init-import branch June 21, 2023 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.

3 participants