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 validation for crate/package name in new/init #1943

Merged
merged 5 commits into from
Feb 27, 2024

Conversation

alonme
Copy link
Contributor

@alonme alonme commented Feb 17, 2024

partially solves #1398 (only cargo)

The current commit only adds a very basic validation as a POC before implementing the needed cargo / pypi checks
A few questions i have before finishing the implementation

  1. The cargo validation seems to be comprised of 2 main parts,
    a. Basic validation which is part of the crate cargo-util-schemas, and is not made public outside the crate

    b. Extra validations that are public from the cargo package

    I believe we should vendor these logics - one is private, and the other requires the cargo package which i believe will add many dependencies to the project

  2. The generate_project function is de-facto defining a default value for GenerateProjectOptions.name - would it be better to move this logic into GenerateProjectOptions?

Copy link

netlify bot commented Feb 17, 2024

Deploy Preview for maturin-guide ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 9b21beb
🔍 Latest deploy log https://app.netlify.com/sites/maturin-guide/deploys/65d9fb0ee8676600086e03ee
😎 Deploy Preview https://deploy-preview-1943--maturin-guide.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@messense
Copy link
Member

  1. I believe we should vendor these logics - one is private, and the other requires the cargo package which i believe will add many dependencies to the project

Agreed.

2. The generate_project function is de-facto defining a default value for GenerateProjectOptions.name - would it be better to move this logic into GenerateProjectOptions?

Makes sense.

name,
);
}
if is_windows_reserved(name) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original code bailed if the platform is actually windows, I think this should be good enough for now

@alonme
Copy link
Contributor Author

alonme commented Feb 24, 2024

@messense I believe this is ready for review 😄

@messense messense self-requested a review February 27, 2024 06:23
@messense messense merged commit dd81231 into PyO3:main Feb 27, 2024
29 checks passed
@alonme
Copy link
Contributor Author

alonme commented Feb 27, 2024

@messense Thanks!

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.

2 participants