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

[SofaPython3] Remove -at-best- the use of this infamous string conver… #45

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

damienmarchal
Copy link
Contributor

…sion while creating an object.

@damienmarchal damienmarchal force-pushed the pr-remove-str-conversion-in-create branch from 6fdc309 to 24c22d3 Compare August 31, 2020 19:55
…h invalid argument.

(this was the previous behavior)
The PR is breaking existing code. But the previously working code was,
despite being conveniant was not valid and it was using a "flat" array as
a multi-dimensionnal structure. It was not noticed before because the lists were
converted into a 1D array then into a big string.

Two options were possibles:
a) not allowing this kind of code (using a 1D array to fill a 2D structure)
b) implement kind of de-flattening a python lists into the corresponding multidimmensional matrix

For the simplicity I choose option (a), so I updated the tests to use the 'right' syntax.
@jnbrunet jnbrunet added this to the V20.12 milestone Nov 9, 2020
@jnbrunet jnbrunet modified the milestones: V20.12, V21.06 Dec 23, 2020
@hugtalbot
Copy link
Contributor

I see this is not active anymore since December, should we close it @damienmarchal ?

@damienmarchal
Copy link
Contributor Author

No it is worth keeping it.
Despite beeing breaking the feature is important. And deserve discussion and an integration roadmap.

@damienmarchal
Copy link
Contributor Author

I think that with the help of PR #184 we now have the proper way to integrate this breaking change in Sofa. Despite the change is breaking a core feature... it will be activate no demand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants