-
Notifications
You must be signed in to change notification settings - Fork 4
Add script: merge concat without space #602
Conversation
tests/common/scripts/merging/test_merge_concat_without_separator.py
Outdated
Show resolved
Hide resolved
from common.scripts.merging import merge_concat_without_separator | ||
|
||
|
||
def test_merge_concat_without_separator(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to separate this into several tests (for each type)
And you can also use parametrization to improve readability and copy-paste errors: https://docs.pytest.org/en/6.2.x/parametrize.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! I'm trying to understand the purpose of this PR, and why you needed to add this script, but I'm probably a step behind the latest developments on cleaning scripts. Didn't this script already exist ? https://github.com/arkhn/cleaning-scripts/blob/master/scripts/utils/concat_without_separator.py
Or did you want to do some cleaning ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be easier to have all the tests in the same file 🙂
The scripts on the utils folder doesn't appear on pyrog, their purpose is to be called by other scripts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if tests do pass 🙂
Fixes
Fixes #[issue number] by @[issue author]
Description
Technical details
Tests