-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Validate user input from init command #10457
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
52aa2a5
2b45f05
7d1e5fd
d5925a3
14646c4
ec68106
8458f45
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,9 +13,11 @@ | |
| from cleo.helpers import option | ||
| from packaging.utils import canonicalize_name | ||
| from tomlkit import inline_table | ||
| from tomlkit import parse | ||
|
|
||
| from poetry.console.commands.command import Command | ||
| from poetry.console.commands.env_command import EnvCommand | ||
| from poetry.factory import Factory | ||
| from poetry.utils.dependency_specification import RequirementsParser | ||
| from poetry.utils.env.python import Python | ||
|
|
||
|
|
@@ -265,6 +267,14 @@ def _init_pyproject( | |
|
|
||
| return 1 | ||
|
|
||
| # Validate fields before creating pyproject.toml file. If any validations fail, throw an error. | ||
| # Convert TOML string to a TOMLDocument (a dict-like object) for validation. | ||
| pyproject_dict = parse(pyproject.data.as_string()) | ||
| validation_results = self._validate(pyproject_dict) | ||
| if validation_results.get("errors"): | ||
| self.line_error(f"<error>Validation failed: {validation_results}</error>") | ||
| return 1 | ||
|
Comment on lines
+274
to
+280
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The output is not so nicely formatted. It contains a raw dict, e.g.: You may take a look at https://github.com/python-poetry/poetry-core/blob/002aa3e16f98d21645bb9a45f698b55adc40f317/src/poetry/core/factory.py#L53-L60 for better formatting.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @radoering , thanks for the feedback about this. Would this format be acceptable?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Absolutely.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @radoering , change made.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You must be kidding. Error messages should be readable and clear for users. Imagine some beginner developer getting hit by that error message. Hell, 9/10 seasoned developers would have a hard time understanding that error message without some sort of regex explorer. This should be a clear message, that a 5 year old is able to understand. I would split the checks and make a separate clear message for each condition, gather the errors and list them clearly. Poetry's motto is "Python packaging and dependency management made easy". Getting hit with a regex is not "easy" by any means.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My comment was just about formatting the output. At this point, we get a dict with lists of messages and the output should just be nicely formatted. The content of the single messages has been created before and is fixed at this point. In my opinion, it is too much effort to improve each possible message that comes from schema validation - or at least this is clearly out of scope of this PR. The message is the same message if you run |
||
|
|
||
| pyproject.save() | ||
|
|
||
| if create_layout: | ||
|
|
@@ -533,3 +543,13 @@ def _get_pool(self) -> RepositoryPool: | |
| self._pool.add_repository(PyPiRepository(pool_size=pool_size)) | ||
|
|
||
| return self._pool | ||
|
|
||
| @staticmethod | ||
| def _validate(pyproject_data: dict[str, Any]) -> dict[str, Any]: | ||
sourcery-ai[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| """ | ||
| Validates the given pyproject data and returns the validation results. | ||
| """ | ||
| # Instantiate a new Factory to avoid relying on shared/global state, | ||
| # which can cause unexpected behavior in other parts of the codebase or test suite. | ||
| factory = Factory() | ||
| return factory.validate(pyproject_data) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Factory.validate(pyproject.data)should suffice.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@radoering , thanks for the suggestion. I updated the code to call
Factory.validate(pyproject.data)directly. All tests are passing!I kept it inside of
InitCommand()._validate()so that I can unit-test it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I see. So we are actually just unit testing
Factory.validate(), which does not give much value because it is (or at least should be) already tested intest_factory.py.Further, my comment suggests that converting
pyproject.datato string and parsing it is unnecessary. You probably also do this just for unit testing to have a clear interface? I do not like that the production code becomes more complicated just for unit testing.I think you should check the other (non-interactive) unit tests in
test_init.pyand use one of these as base for your unit test so that you do not need a separate method to test. Further, this is probably the wrong place to test the regex in detail because as mentioned the validation is part ofFactory. Here, we should just check that validation is called and gives a nicely formatted output.