Skip to content

Conversation

@jordanjeremy
Copy link

Current csv string creation can produce invalid strings in the event of something like the delimiter being present in a value. Propose using the csv module to create these strings instead of just concatenating values together, to ensure correct strings are created.

Adds argument for csv_dialect to allow user to provide settings for all possible formatting options. If not provided will create dialect based on existing value_separator and line_separator arguments, so they will be used in the event that a user has passed custom values.

Ensures cases like delimiter being in values are handled
correctly.
@MariusWirtz
Copy link
Collaborator

Nice work @jordanjeremy. Thank you.

I will do testing tonight and provide feedback if I find anything.

@MariusWirtz
Copy link
Collaborator

There is a small difference between the new implementation and the old one.
The new one includes a linebreak in the last row. The old one didn't.

If it's a small change it would be nice to comply with the old implementation.
Or is there perhaps a good reason to deviate from the old way?

By the way, we have some test cases defined for the CellService class. You can find them here:
https://github.com/cubewise-code/tm1py/blob/master/Tests/CellService_test.py
You can find a small readme on the tests here:
https://github.com/cubewise-code/tm1py/blob/master/Tests/_readme.md

Currently, 7 tests fail due to the two issues I raised above.

image

@jordanjeremy
Copy link
Author

@MariusWirtz that you for the feedback. I will look into the issues identified and follow up.

Correct error raised if include_attributes enabled.
Remove trailing new line from string.
@MariusWirtz
Copy link
Collaborator

The changes look good and all tests pass.

Thanks for the contribution @jordanjeremy!

@MariusWirtz MariusWirtz merged commit 276d74f into cubewise-code:master Jan 28, 2022
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