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

Changed to use list instead of string for subprocess command. #214

Merged
merged 1 commit into from
May 7, 2024

Conversation

j-emils
Copy link
Contributor

@j-emils j-emils commented May 7, 2024

Related Issues

Purpose

Cannot execute the simulate method on linux.

Approach

Adapting subprocess input.

@CLAassistant
Copy link

CLAassistant commented May 7, 2024

CLA assistant check
All committers have signed the CLA.

@arun3688
Copy link
Collaborator

arun3688 commented May 7, 2024

@j-emils Thanks for the fix

@arun3688 arun3688 closed this May 7, 2024
@arun3688 arun3688 reopened this May 7, 2024
@arun3688 arun3688 merged commit 35a67ed into OpenModelica:master May 7, 2024
3 checks passed
@arun3688
Copy link
Collaborator

arun3688 commented May 7, 2024

@j-emils Thanks for the fix

@lafrech
Copy link

lafrech commented Jun 10, 2024

I came to report this until I realized it was fixed in master branch already. Thanks @j-emils for the fix.

A proper resolution would be to create the list in the first place rather than concatenate strings then split, but oh well.

Also, I believe the list should be split one step further to separate e.g. -r and the file path.

See for instance Popen docs:

    Popen(["/usr/bin/git", "commit", "-m", "Fixes a bug."])

although I can't comment on the consequences of not doing it.

I did it like this as a test but really, it would be more sensible to build the list in the first place.

cmd = sum([r.split("=") for r in cmd.split()], [])

@arun3688 considering the fixes that have been merged since last release, a new release would be greatly appreciated.

Thanks!

@casella
Copy link

casella commented Jun 10, 2024

@j-emils do you mean you'd like to get this fix in OpenModlica 1.23.x?

@lafrech
Copy link

lafrech commented Jun 11, 2024 via email

@arun3688 arun3688 mentioned this pull request Jun 11, 2024
@arun3688
Copy link
Collaborator

@j-emils A new release has been made with v.3.5.2 and also the pip package has been updated with the new release pip install OMPython==3.5.2

@lafrech
Copy link

lafrech commented Jun 11, 2024 via email

@j-emils
Copy link
Contributor Author

j-emils commented Jun 12, 2024

@lafrech, I agree that a more thorough solution would be to create the list from the get go. However, the focus of this fix was to resolve the immediate issue of the code not working. In the future there is a need for refactoring the code in my opinion.

@casella
Copy link

casella commented Jun 12, 2024

@lafrech, I agree that a more thorough solution would be to create the list from the get go. However, the focus of this fix was to resolve the immediate issue of the code not working. In the future there is a need for refactoring the code in my opinion.

You may open another ticket for that, so we don't forget about it.

@lafrech
Copy link

lafrech commented Jun 12, 2024

@j-emils I totally understand and agree
@casella done: #217

Have a nice day, guys.

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.

5 participants