Skip to content

fixed the issues where an empty property will cause a crash due to index out of range#2066

Closed
mourra950 wants to merge 3 commits intocookiecutter:mainfrom
mourra950:main
Closed

fixed the issues where an empty property will cause a crash due to index out of range#2066
mourra950 wants to merge 3 commits intocookiecutter:mainfrom
mourra950:main

Conversation

@mourra950
Copy link
Copy Markdown

as described in the issue #2065 i have tried to manually fix it by checking the len of the array before returning the first element and avoid crashing other wise return None

Comment on lines 276 to 281
if no_input:
return rendered_options[0]
if len(rendered_options) != 0:
return rendered_options[0]
else:
return None
return read_user_choice(key, rendered_options, prompts, prefix)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This cannot return None -- that contradicts the return type annotation (OrderedDict or str).

This should be formulated as:

    if not rendered_options:
        raise ValueError
    if no_input:
        return rendered_options[0]
    return read_user_choice(key, rendered_options, prompts, prefix)

read_user_choice() has a condition to ensure that the list of options isn't empty, but we can catch that here.

This also needs to have a test added to the test suite so that this issue is not simply fixed, but provably and permanently fixed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I ran into the same issue where an empty list raises an exception and kills the process. Would you accept this solution if it returned an empty string rather than None? and tests of course

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@meganlkm I don't have the ability to merge PRs in this repo, I can only review PRs.

It's been over a year since I provided feedback on this so I'm closing the PR, but if you're hitting this issue and have the ability to submit a PR, please do!

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.

3 participants