Fix: Misconfiguration in ask dependency interactive by PabloEmidio · Pull Request #7569 · python-poetry/poetry

PabloEmidio · 2023-02-27T22:48:52Z

Fixing a misunderstand of buitin str function: maxsplit can return maxsplit+1 items

Resolves: #7567

Although, this alteration resolves the broken message reported, we'd be better to treat the string using regex and validating the received result.

radoering · 2023-02-28T05:21:49Z

Can you add a test, please?

PabloEmidio · 2023-03-01T01:08:53Z

@radoering debbuging the source code, I find the follow behavior as used this PR change:

Package to add or search for (leave blank to skip): now we crash
> /home/utrape/Documents/programming/contributions/poetry/src/poetry/console/commands/init.py(484)_validate_package()
-> if package and len(package.split()) > 2:
(Pdb) c
Invalid package definition.
Package to add or search for (leave blank to skip): about now
> /home/utrape/Documents/programming/contributions/poetry/src/poetry/console/commands/init.py(484)_validate_package()
-> if package and len(package.split()) > 2:
(Pdb) c
Adding about now

Add a package (leave blank to skip): here we go
Adding here we go

Clearly the validation has not been called at some asking.

Working on it.

PabloEmidio · 2023-03-01T01:59:23Z

I modify an existing test to add the scenary reported. When runned against master generate the follow output:

=========================================================================== FAILURES ============================================================================
_________________________________________________________ test_interactive_with_wrong_dependency_inputs _________________________________________________________
[gw3] linux -- Python 3.11.2 /home/utrape/.cache/pypoetry/virtualenvs/poetry-X0uBKQkB-py3.11/bin/python

tester = <cleo.testers.command_tester.CommandTester object at 0x7f2628c88ed0>, repo = <tests.helpers.TestRepository object at 0x7f2628c49950>

    def test_interactive_with_wrong_dependency_inputs(
        tester: CommandTester, repo: TestRepository
    ):
        repo.add_package(get_package("pendulum", "2.0.0"))
        repo.add_package(get_package("pytest", "3.6.0"))
    
        inputs = [
            "my-package",  # Package name
            "1.2.3",  # Version
            "This is a description",  # Description
            "n",  # Author
            "MIT",  # License
            "^3.8",  # Python
            "",  # Interactive packages
            "foo 1.19.2",
            "pendulum 2.0.0 foo",  # Package name and constraint (invalid)
            "pendulum 2.0.0",  # Package name and constraint (invalid)
            "pendulum 2.0.0",  # Package name and constraint (invalid)
            "pendulum 2.0.0",  # Package name and constraint (invalid)
            "pendulum@^2.0.0",  # Package name and constraint (valid)
            "",  # End package selection
            "",  # Interactive dev packages
            "pytest 3.6.0 foo",  # Dev package name and constraint (invalid)
            "pytest 3.6.0",  # Dev package name and constraint (invalid)
            "[email protected]",  # Dev package name and constraint (valid)
            "",  # End package selection
            "\n",  # Generate
        ]
>       tester.execute(inputs="\n".join(inputs))

/home/utrape/Documents/programming/contributions/poetry/tests/console/commands/test_init.py:623: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
/home/utrape/.cache/pypoetry/virtualenvs/poetry-X0uBKQkB-py3.11/lib/python3.11/site-packages/cleo/testers/command_tester.py:88: in execute
    self._status_code = self._command.run(self._io)
/home/utrape/.cache/pypoetry/virtualenvs/poetry-X0uBKQkB-py3.11/lib/python3.11/site-packages/cleo/commands/base_command.py:119: in run
    status_code = self.execute(io)
/home/utrape/.cache/pypoetry/virtualenvs/poetry-X0uBKQkB-py3.11/lib/python3.11/site-packages/cleo/commands/command.py:62: in execute
    return self.handle()
/home/utrape/Documents/programming/contributions/poetry/src/poetry/console/commands/init.py:207: in handle
    self._format_requirements(self._determine_requirements([]))
/home/utrape/Documents/programming/contributions/poetry/src/poetry/console/commands/init.py:297: in _determine_requirements
    constraint = self._parse_requirements([package])[0]
/home/utrape/Documents/programming/contributions/poetry/src/poetry/console/commands/init.py:438: in _parse_requirements
    return [
/home/utrape/Documents/programming/contributions/poetry/src/poetry/console/commands/init.py:439: in <listcomp>
    parse_dependency_specification(
/home/utrape/Documents/programming/contributions/poetry/src/poetry/utils/dependency_specification.py:218: in parse_dependency_specification
    or _parse_dependency_specification_simple(requirement)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

requirement = 'pendulum 2.0.0 foo'

    def _parse_dependency_specification_simple(
        requirement: str,
    ) -> DependencySpec | None:
        extras: list[str] = []
        pair = re.sub("^([^@=: ]+)(?:@|==|(?<![<>~!])=|:| )(.*)$", "\\1 \\2", requirement)
        pair = pair.strip()
    
        require: DependencySpec = {}
    
        if " " in pair:
>           name, version = pair.split(" ", 2)
E           ValueError: too many values to unpack (expected 2)

/home/utrape/Documents/programming/contributions/poetry/src/poetry/utils/dependency_specification.py:117: ValueError

src/poetry/console/commands/init.py

tests/console/commands/test_init.py

Fixing a misunderstand of buitin str function

Add scenary to bug reported in python-poetry#7567

PabloEmidio · 2023-03-03T14:07:36Z

Could anyone give any thought about it's something missing or it can be merged?

radoering · 2023-03-03T14:52:40Z

Since there is a fix in parse_dependency_specification it might make sense to extend test_parse_dependency_specification.

PabloEmidio · 2023-03-03T15:16:10Z

Since there is a fix in parse_dependency_specification it might make sense to extend test_parse_dependency_specification.

@radoering the fix in parse_dependency_specification was to prevent the error too many values to unpack (expected 2) saw in the issues to happen. But the way it was handle allows the follow behavior (using the pytest.mark.parametrize of test_parse_dependency_specification to illustrate)

        ("demo 1.0.0", {"name": "demo", "version": "1.0.0"}),
        ("demo 1.0.0 foo", {"name": "demo", "version": "1.0.0 foo"}),

Maybe it'll be better to check the requirement against the number of argument passed in the shell:

name, version, *_excess = pair.split(" ", 2)
if _excess:
    raise ValueError(f"Not recognize argument(s) {_excess}")
...

And add some test to validate this raise given the context.

What do you think?

radoering · 2023-03-03T15:35:25Z

I think poetry should not crash with a ValueError. (I didn't check if such an error is caught somewhere.) The best response would be to tell the user that the dependency specification is invalid and ask again for a package.

PabloEmidio · 2023-03-03T15:55:19Z

I think poetry should not crash with a ValueError. (I didn't check if such an error is caught somewhere.) The best response would be to tell the user that the dependency specification is invalid and ask again for a package.

@radoering, I suggest the ValueError because the functions that called this code raise ValueError in x scenarios.

We could catch the ValueError in the _determine_requirements and ask again:

# src/poetry/console/commands/init.py:L302
try:
    constraint = self._parse_requirements([package])[0]
except ValueError as err:
    self.line_error(f"<error>{str(err)}</error>")
    package = self.ask(question)
    continue

radoering · 2023-03-03T16:43:04Z

It seems _validate_package already handles this. If I provoke an Invalid package definition., poetry asks for the next package. Maybe, we can just do the complete parsing, i.e. call _parse_requirements, in _validate_package?

PabloEmidio · 2023-03-03T16:56:02Z

@radoering exactly. The central idea is the requirement already was validate before been passed to parse_dependency_specification. But, it has been the central idea for while and we got 2 issue about it. And anyhow, the quote bellow continue to be true and the right way to split in max 2 items is passing maxsplit=1.

Fixing a misunderstand of buitin str function: maxsplit can return maxsplit+1 items

PabloEmidio · 2023-03-03T17:02:43Z

looking the application as a whole, the currently code is going to work property. But looking only to _parse_dependency_specification_simple and knowing what return expect, the function does not attend its purpose.

radoering

Now, I realize that due to the fix in init.py the fix in dependency_specification.py does not matter anymore (but is more correct nevertheless) because there will always be just one space.

With that knowledge I think this PR is fine as is.

(cherry picked from commit a58a0e8)

github-actions · 2024-03-03T14:06:31Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

PabloEmidio changed the title ~~Fix: Change mistaken argument passed in str.split~~ Fix: Misconfiguration in ask dependency interactive Mar 1, 2023

dimbleby reviewed Mar 1, 2023

View reviewed changes

src/poetry/console/commands/init.py Outdated Show resolved Hide resolved

dimbleby reviewed Mar 1, 2023

View reviewed changes

tests/console/commands/test_init.py Show resolved Hide resolved

PabloEmidio requested review from dimbleby and neersighted and removed request for dimbleby and neersighted March 1, 2023 17:58

PabloEmidio added 5 commits March 1, 2023 17:47

Fix: Change mistaken argument passed in str.split

23d3e16

Fixing a misunderstand of buitin str function

Fix: make sure all interactive dependency is being validated

071ccd7

Tests: Modify test to add another validate

df53325

Add scenary to bug reported in python-poetry#7567

Test(refactor): Cleaning unnecessary oversight code up

ce184c0

Fix: Declare and use a follow up question to separate contexts

b2f31fe

PabloEmidio force-pushed the patch-1 branch from bcffcdf to b2f31fe Compare March 1, 2023 20:47

radoering approved these changes Mar 4, 2023

View reviewed changes

radoering merged commit a58a0e8 into python-poetry:master Mar 4, 2023

radoering added area/cli Related to the command line impact/backport Requires backport to stable branch backport/1.4 labels Mar 4, 2023

poetry-bot bot mentioned this pull request Mar 4, 2023

[1.4] Fix: Misconfiguration in ask dependency interactive #7602

Merged

poetry-bot bot pushed a commit that referenced this pull request Mar 4, 2023

Fix: Misconfiguration in ask dependency interactive (#7569)

ffec6d5

(cherry picked from commit a58a0e8)

PabloEmidio deleted the patch-1 branch March 4, 2023 13:09

radoering pushed a commit that referenced this pull request Mar 4, 2023

Fix: Misconfiguration in ask dependency interactive (#7569)

67f16d8

(cherry picked from commit a58a0e8)

radoering mentioned this pull request Mar 19, 2023

release: bump version to 1.4.1 #7683

Merged

github-actions bot locked as resolved and limited conversation to collaborators Mar 3, 2024

Comments

Conversation

PabloEmidio commented Feb 27, 2023

Uh oh!

radoering commented Feb 28, 2023

Uh oh!

PabloEmidio commented Mar 1, 2023

Uh oh!

PabloEmidio commented Mar 1, 2023

Uh oh!

Uh oh!

Uh oh!

PabloEmidio commented Mar 3, 2023

Uh oh!

radoering commented Mar 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PabloEmidio commented Mar 3, 2023

Uh oh!

radoering commented Mar 3, 2023

Uh oh!

PabloEmidio commented Mar 3, 2023

Uh oh!

radoering commented Mar 3, 2023

Uh oh!

PabloEmidio commented Mar 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PabloEmidio commented Mar 3, 2023

Uh oh!

radoering left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

radoering commented Mar 3, 2023 •

edited

Loading

PabloEmidio commented Mar 3, 2023 •

edited

Loading