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

Fix --tools=all not working properly #3361

Merged
merged 29 commits into from
Dec 8, 2023
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
5faebf3
add fix
SajidAlamQB Nov 29, 2023
0f61dd5
Merge branch 'develop' into bugfix/addons=all-fix
lrcouto Nov 29, 2023
7e7ca3f
add fix
SajidAlamQB Nov 29, 2023
16ef432
Rename addons to tools (#3357)
AhdraMeraliQB Nov 29, 2023
87e2e1e
Fix broken tests (#3366)
AhdraMeraliQB Nov 29, 2023
9ab8759
Merge branch 'develop' into bugfix/addons=all-fix
lrcouto Nov 30, 2023
f127b48
Fix variable names on _convert_tool_names_to_numbers
lrcouto Nov 30, 2023
4090586
Merge branch 'bugfix/addons=all-fix' of github.com:kedro-org/kedro in…
lrcouto Nov 30, 2023
47c4118
Merge branch 'bugfix/addons=all-fix' of https://github.com/kedro-org/…
SajidAlamQB Nov 30, 2023
70985cc
Merge branch 'develop' into bugfix/addons=all-fix
SajidAlamQB Nov 30, 2023
a3eea61
Merge branch 'develop' into bugfix/addons=all-fix
SajidAlamQB Nov 30, 2023
fd7b734
Merge branch 'develop' into bugfix/addons=all-fix
lrcouto Nov 30, 2023
6888463
Merge branch 'develop' into bugfix/addons=all-fix
lrcouto Nov 30, 2023
143b204
Merge branch 'develop' into bugfix/addons=all-fix
lrcouto Nov 30, 2023
ba0d696
Add tests for helper functions
lrcouto Dec 1, 2023
dab4d29
Merge branch 'develop' into bugfix/addons=all-fix
SajidAlamQB Dec 5, 2023
d6672a3
Merge branch 'develop' into bugfix/addons=all-fix
lrcouto Dec 5, 2023
f9802c7
Merge branch 'develop' into bugfix/addons=all-fix
lrcouto Dec 6, 2023
b353f63
Merge branch 'develop' into bugfix/addons=all-fix
lrcouto Dec 6, 2023
545755a
Lint
lrcouto Dec 6, 2023
4bc1748
Merge branch 'develop' into bugfix/addons=all-fix
lrcouto Dec 6, 2023
93d5b50
Merge branch 'develop' into bugfix/addons=all-fix
lrcouto Dec 7, 2023
c875b3c
Merge branch 'develop' into bugfix/addons=all-fix
lrcouto Dec 7, 2023
3df74ab
Return numbers to tools names on conversion from all
lrcouto Dec 7, 2023
a821ede
Merge branch 'bugfix/addons=all-fix' of github.com:kedro-org/kedro in…
lrcouto Dec 7, 2023
d33b745
Merge branch 'develop' into bugfix/addons=all-fix
lrcouto Dec 7, 2023
a228843
Merge branch 'develop' into bugfix/addons=all-fix
SajidAlamQB Dec 8, 2023
31d7a3f
Merge branch 'develop' into bugfix/addons=all-fix
SajidAlamQB Dec 8, 2023
3e6666e
Merge branch 'develop' into bugfix/addons=all-fix
SajidAlamQB Dec 8, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions kedro/framework/cli/starters.py
Original file line number Diff line number Diff line change
Expand Up @@ -489,6 +489,10 @@ def _convert_tool_names_to_numbers(selected_tools: str | None) -> str | None:
"""
if selected_tools is None:
return None
if selected_tools.lower() == "none":
return ""
if selected_tools.lower() == "all":
return ",".join(TOOLS_SHORTNAME_TO_NUMBER.values())
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we just need to update the TOOLS_SHORTNAME_TO_NUMBER dictionary? Put "none" and "all" there? Or it will break something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking this as well but it feels a bit weird as "all" and "none" aren't tools.

merelcht marked this conversation as resolved.
Show resolved Hide resolved

tools = []
for tool in selected_tools.lower().split(","):
Expand Down
89 changes: 89 additions & 0 deletions tests/framework/cli/test_starters.py
Original file line number Diff line number Diff line change
Expand Up @@ -1327,3 +1327,92 @@ def test_invalid_names(self, fake_kedro_cli, name):
"Kedro project names must contain only alphanumeric symbols, spaces, underscores and hyphens and be at least 2 characters long"
in result.output
)


class TestParseYesNoToBools:
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we move the tests for parse_tools_input into their own class as well? For consistency

@pytest.mark.parametrize(
"input",
["yes", "YES", "y", "Y", "yEs"],
)
def parse_yes_no_to_bool_responds_true(self, input):
assert _parse_yes_no_to_bool(input) is True

@pytest.mark.parametrize(
"input",
["no", "NO", "n", "N", "No", ""],
)
def parse_yes_no_to_bool_responds_false(self, input):
assert _parse_yes_no_to_bool(input) is False

def parse_yes_no_to_bool_responds_none(self):
assert _parse_yes_no_to_bool(None) is None


class TestValidateSelection:
def test_validate_selection_valid(self):
tools = ["1", "2", "3", "4"]
assert _validate_selection(tools) is None

def test_validate_selection_invalid_single_tool(self):
tools = ["8"]
with pytest.raises(SystemExit):
_validate_selection(tools)
Copy link
Contributor

@AhdraMeraliQB AhdraMeraliQB Dec 5, 2023

Choose a reason for hiding this comment

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

Could we add an assert for the correct error message? E.g the implementation for test_parse_tools_invalid_selection


def test_validate_selection_invalid_multiple_tools(self):
tools = ["8", "10", "15"]
with pytest.raises(SystemExit):
_validate_selection(tools)

def test_validate_selection_mix_valid_invalid_tools(self):
tools = ["1", "8", "3", "15"]
with pytest.raises(SystemExit):
_validate_selection(tools)

def test_validate_selection_empty_list(self):
tools = []
assert _validate_selection(tools) is None


class TestConvertToolNamesToNumbers:
def test_convert_tool_names_to_numbers_with_valid_tools(self):
selected_tools = "lint,test,docs"
result = _convert_tool_names_to_numbers(selected_tools)
assert result == "1,2,4"

def test_convert_tool_names_to_numbers_with_none(self):
result = _convert_tool_names_to_numbers(None)
assert result is None

def test_convert_tool_names_to_numbers_with_empty_string(self):
selected_tools = ""
result = _convert_tool_names_to_numbers(selected_tools)
assert result == ""

def test_convert_tool_names_to_numbers_with_none_string(self):
selected_tools = "none"
result = _convert_tool_names_to_numbers(selected_tools)
assert result == ""

def test_convert_tool_names_to_numbers_with_all_string(self):
result = _convert_tool_names_to_numbers("all")
assert result == "1,2,3,4,5,6,7"

def test_convert_tool_names_to_numbers_with_mixed_valid_invalid_tools(self):
selected_tools = "lint,invalid_tool,docs"
result = _convert_tool_names_to_numbers(selected_tools)
assert result == "1,4"

def test_convert_tool_names_to_numbers_with_whitespace(self):
selected_tools = " lint , test , docs "
result = _convert_tool_names_to_numbers(selected_tools)
assert result == "1,2,4"

def test_convert_tool_names_to_numbers_with_case_insensitive_tools(self):
selected_tools = "Lint,TEST,Docs"
result = _convert_tool_names_to_numbers(selected_tools)
assert result == "1,2,4"

def test_convert_tool_names_to_numbers_with_invalid_tools(self):
selected_tools = "invalid_tool1,invalid_tool2"
result = _convert_tool_names_to_numbers(selected_tools)
assert result == ""
Loading