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.

Sign up for GitHub

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

Jump to bottom

[SofaPython/core] forward sys.argv to python scripts #368

Merged
merged 9 commits into from Aug 31, 2017
Merged

[SofaPython/core] forward sys.argv to python scripts #368

merged 9 commits into from Aug 31, 2017

Conversation

maxime-tournier
Copy link
Contributor

@maxime-tournier maxime-tournier commented Aug 21, 2017 •
edited by hugtalbot

Changelog:

  • added a mechanism to obtain extra args from command-line (everything following --argv)
  • forwarded extra args to python in SceneLoaderPy

Example:

# test.py
def createScene(node):
    import sys
    print(sys.argv)
runSofa -a test.py --argv --spam --bacon eggs
# ['test', '--spam', '--bacon', 'eggs']

This PR:

  • builds with SUCCESS for all platforms on the CI.
  • does not generate new warnings.
  • does not generate new unit test failures.
  • does not generate new scene test failures.
  • does not break API compatibility.
  • is more than 1 week old (or has fast-merge label).

Reviewers will merge only if all these checks are true.

@maxime-tournier maxime-tournier mentioned this pull request Aug 21, 2017
Closed
@damienmarchal damienmarchal added the enhancement About a possible enhancement label Aug 21, 2017
guparan
guparan reviewed Aug 22, 2017
View reviewed changes
SofaKernel/framework/sofa/helper/ArgumentParser.cpp
std::cout << "(short name, long name, description, default value)\n-h,\t--help: this help" << std::endl;
std::cout << std::boolalpha;
for( ArgVec::const_iterator a=commands.begin(), aend=commands.end(); a!=aend; ++a )
(*a)->print();
std::cout << std::noboolalpha;

std::cout << "--argv [...]\t" << "forward extra args to the python interpreter" << std::endl;
Copy link
Contributor

@guparan guparan Aug 22, 2017 •
edited

Choose a reason for hiding this comment

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

This help message makes me think --argv is useful only with python. Isn't it something more generic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The PR started from discussion in #356, so I sticked to that. I don't quite see where else it could be useful but if needed, extra arguments can be accessed via the static function ArgumentParser::extra_args()

@guparan
Copy link
Contributor

guparan commented Aug 22, 2017

Warning: there are two failing unit tests.

@maxime-tournier
Copy link
Contributor Author

maxime-tournier commented Aug 25, 2017 •
edited

Note: empty ArgumentParser::extra_args() will not set sys.argv, as opposed to set it some empty list (this was the reason why the test was failing: it would reset it to empty on script loading after being set initially from the test binary).

I'm not quite happy with this kind of side-effect, but this matches the previous behaviour so ¯\_(ツ)_/¯

@hugtalbot hugtalbot added the pr: status to review To notify reviewers to review this pull-request label Aug 28, 2017
@maxime-tournier
Copy link
Contributor Author

build is good, tests are good, gentle bump :-)

@hugtalbot hugtalbot added pr: status ready Approved a pull-request, ready to be squashed and removed pr: status to review To notify reviewers to review this pull-request labels Aug 30, 2017
@hugtalbot
Copy link
Contributor

[ci-build]

@hugtalbot hugtalbot merged commit da84f5f into sofa-framework:master Aug 31, 2017
@maxime-tournier maxime-tournier deleted the sofapython-forward-argv branch August 31, 2017 12:17
matthieu-nesme pushed a commit to Anatoscope/sofa that referenced this pull request Sep 6, 2017
@matthieu-nesme
Merge pull request sofa-framework#368 from Anatoscope/sofapython-forw…
1de1096 
…ard-argv

[SofaPython/core] forward sys.argv to python scripts
(cherry picked from commit da84f5f)

# Conflicts:
#	SofaKernel/framework/sofa/helper/ArgumentParser.cpp
@guparan guparan modified the milestones: v17.06, v17.12 Sep 13, 2017
@guparan guparan added this to Done in User Experience Apr 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guparan guparan guparan left review comments

@damienmarchal damienmarchal Awaiting requested review from damienmarchal

Assignees
No one assigned
Labels
enhancement About a possible enhancement pr: status ready Approved a pull-request, ready to be squashed
Projects
User Experience
  
Done
SofaPython
  
Awaiting triage
Milestone
v17.12
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@maxime-tournier @guparan @hugtalbot @damienmarchal