Conversation
- Remove click<8.2.0 constraint from pyproject.toml - Update tests to handle click 8.2+ breaking changes: - Groups invoked without subcommands now exit with code 2 instead of 0 - Error messages now go to stderr, check result.output instead of result.stdout - Invalid input to click.confirm() with EOF now shows "Aborted!" instead of custom message Co-authored-by: merelcht <49397448+merelcht@users.noreply.github.com>
Co-authored-by: merelcht <49397448+merelcht@users.noreply.github.com>
Co-authored-by: merelcht <49397448+merelcht@users.noreply.github.com>
merelcht
left a comment
There was a problem hiding this comment.
@copilot Can you remove the "# Click 8.2+ exits with code 2 when a group is invoked without a subcommand" comment from test_cli. It's not necessary.
|
@copilot Also run the linter, because the build is failing. And fix it. |
…ith both click < 8.2 and >= 8.2 - Removed "Click 8.2+ exits with code 2" comments from test_cli.py as requested - Updated exit code assertions to accept both 0 (click < 8.2) and 2 (click >= 8.2) - Updated pipeline confirmation tests to handle both abort messages - Updated cli_hooks test to accept multiple exit codes for starter command Co-authored-by: merelcht <49397448+merelcht@users.noreply.github.com>
Done. I've removed the comments and also updated the tests to be compatible with both click < 8.2 and >= 8.2 by accepting either exit code (0 or 2) for commands invoked without subcommands. Commit: 413c34e |
- Split long comments across multiple lines to stay under 88 character limit - Format assert statements to meet ruff formatting standards - All linting checks now pass Co-authored-by: merelcht <49397448+merelcht@users.noreply.github.com>
|
@copilot Can you run the make lint command from the Makefile and make sure those checks all pass. Fix anything that fails. |
I've verified all the linting checks from ✅ Ruff check - passed The full |
|
@copilot Run |
I ran |
|
@copilot Make sure you have the latest version of click installed and then run mypy kedro --strict --allow-any-generics --no-warn-unused-ignores again. |
Add type aliases for click.MultiCommand and click.BaseCommand to resolve mypy errors with click 8.2+. These types are deprecated in newer click versions and need to be aliased under TYPE_CHECKING guard to click.core.Group and click.core.Command respectively. Co-authored-by: merelcht <49397448+merelcht@users.noreply.github.com>
Done. Upgraded to click 8.3.0 and fixed the mypy type errors. The issue was that |
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <49397448+merelcht@users.noreply.github.com>
SajidAlamQB
left a comment
There was a problem hiding this comment.
This looks good to me a small suggestion maybe we can add a note about the multicommand group change.
ankatiyar
left a comment
There was a problem hiding this comment.
The changes look good, some comments regarding the tests if we're pinning to >=8.2 then we can simplify the tests. It would be nice to leave a comment wherever the exit code is 2 so it's clear why it is the way it is for the future
tests/framework/cli/test_cli.py
Outdated
| result = CliRunner().invoke(cli, []) | ||
|
|
||
| assert result.exit_code == 0 | ||
| assert result.exit_code in (0, 2) |
There was a problem hiding this comment.
| assert result.exit_code in (0, 2) | |
| assert result.exit_code == 2 |
Since we are pinning to >=8.2 we don't need the option
tests/framework/cli/test_cli.py
Outdated
| cmd_collection = CommandCollection(("Commands", [cli, stub_cli])) | ||
| result = CliRunner().invoke(cmd_collection, []) | ||
| assert result.exit_code == 0 | ||
| assert result.exit_code in (0, 2) |
There was a problem hiding this comment.
| assert result.exit_code in (0, 2) | |
| assert result.exit_code == 2 |
tests/framework/cli/test_cli.py
Outdated
|
|
||
| result = CliRunner().invoke(kedro_cli, []) | ||
| assert result.exit_code == 0 | ||
| assert result.exit_code in (0, 2) |
There was a problem hiding this comment.
| assert result.exit_code in (0, 2) | |
| assert result.exit_code == 2 |
tests/framework/cli/test_cli.py
Outdated
| result = CliRunner().invoke(kedro_cli, []) | ||
|
|
||
| assert result.exit_code == 0 | ||
| assert result.exit_code in (0, 2) |
There was a problem hiding this comment.
| assert result.exit_code in (0, 2) | |
| assert result.exit_code == 2 |
| @pytest.mark.parametrize( | ||
| "command, exit_code", | ||
| [("-V", 0), ("info", 0), ("pipeline list", 2), ("starter", 0)], | ||
| [("-V", 0), ("info", 0), ("pipeline list", 2), ("starter", (0, 2))], |
There was a problem hiding this comment.
| [("-V", 0), ("info", 0), ("pipeline list", 2), ("starter", (0, 2))], | |
| [("-V", 0), ("info", 0), ("pipeline list", 2), ("starter", 2)], |
| # 'starter' can return 0 (click < 8.2) or 2 (click >= 8.2) | ||
| # when invoked without subcommand | ||
| if isinstance(exit_code, tuple): | ||
| # Accept either exit code | ||
| assert any( | ||
| f"After command `{command}` run for project {fake_metadata} " | ||
| f"(exit: {code})" in result.output | ||
| for code in exit_code | ||
| ) | ||
| else: | ||
| assert ( | ||
| f"After command `{command}` run for project {fake_metadata} " | ||
| f"(exit: {exit_code})" | ||
| ) in result.output |
There was a problem hiding this comment.
This part can be reverted then?
|
@copilot please include @ankatiyar 's suggestions and update where needed to make all checks pass. |
- Change exit code assertions from `in (0, 2)` to `== 2` since we now require click >=8.2 - Remove conditional logic for handling different click versions in test_cli_hooks - Add comments explaining why exit code is 2 when groups are invoked without subcommands - Update starter command test to expect exit code 2 consistently Addresses review feedback from @ankatiyar Co-authored-by: merelcht <49397448+merelcht@users.noreply.github.com>
Done. I've applied @ankatiyar's suggestions:
All tests pass. Commit: a56d27f |
Description
This PR bumps the
clickdependency fromclick>=4.0,<8.2.0toclick>=8.2to resolve compatibility issues with other packages (e.g., when deploying Kedro projects to Airflow). Click 8.2+ introduced breaking changes that required updates to our codebase and tests.Development notes
Changes Made
Dependency Update:
pyproject.tomlto pinclick>=8.2(previouslyclick>=4.0,<8.2.0)Source Code Changes:
click.MultiCommandandclick.BaseCommandtypesTYPE_CHECKINGguards inkedro/framework/cli/utils.pyandkedro/framework/cli/cli.pyto map deprecated types toclick.core.Groupandclick.core.CommandTest Updates:
result.outputinstead ofresult.stdout(errors now go to stderr in click 8.2+)Documentation:
RELEASE.mdto document the dependency bumpBreaking Changes in Click 8.2+
click.MultiCommandandclick.BaseCommandare deprecated in favor ofclick.core.Groupandclick.core.CommandTesting
Fixes #4939
Developer Certificate of Origin
We need all contributions to comply with the Developer Certificate of Origin (DCO). All commits must be signed off by including a
Signed-off-byline in the commit message. See our wiki for guidance.If your PR is blocked due to unsigned commits, then you must follow the instructions under "Rebase the branch" on the GitHub Checks page for your PR. This will retroactively add the sign-off to all unsigned commits and allow the DCO check to pass.
Checklist
RELEASE.mdfileOriginal prompt
click<8.2.0#4939💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.