Skip to content

Conversation

johnWilshire
Copy link

The default splitting splits the description and only takes the first part so we need to join the trailing parts dropped by the zip, when passed lists with different lengths.

Copy link
Member

@12rambau 12rambau left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your contribution ! Could you share a small script that showcase this use case so we can add it to the tests ? and also I'm not sure it's necessary to add so many lines to cover your problem and the default behaviour, the join could be applied to all results and only affect description as the others are single element lists.

Comment on lines +97 to +99
fix_description = False
if headers[-1] == "Description":
fix_description = True
Copy link
Member

Choose a reason for hiding this comment

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

could be done in 1 line:

Suggested change
fix_description = False
if headers[-1] == "Description":
fix_description = True
fix_description = headers[-1] == "Description"

parts = line.split()
part_result = dict(zip(headers, parts))

if fix_description:
Copy link
Member

Choose a reason for hiding this comment

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

the evaluation of the fix_description could actually be done directly in the if statement with the comment associated so it's clear it's only necessary there.

part_result = dict(zip(headers, parts))

if fix_description:
part_result["Description"] = " ".join(parts[len(headers) - 1 :])
Copy link
Member

Choose a reason for hiding this comment

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

actually open question, if we join them all, the other should not be affected as they are a 1 item list right ? So you might be able to simply add this mechanism to all part_results instead of just the description, avoiding the all if statement in the process.

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.

2 participants