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

fixing argparsing of planet file #1215

Merged
merged 2 commits into from
Jan 9, 2025
Merged

Conversation

T-Wainwright
Copy link
Contributor

Minor bug fix- looks like there's a name mismatch of the argparse definition and namespace name of the planet file. Currently python doesn't work as is.

@agodemar
Copy link
Contributor

agodemar commented Jan 8, 2025

@bcoconni do you approve this?

@bcoconni
Copy link
Member

bcoconni commented Jan 8, 2025

Currently python doesn't work as is.

Could you be more specific about that ? Because I don't see what this change is actually fixing:

  • Scripts are passed via the argument --script, not --scriptfile.
  • Aircraft is passed via the argument --aircraft, not --aircraftfile.

and both are reported to require a filename in the metavar= parameter.

jsbsim/python/JSBSim.py

Lines 42 to 45 in 5be7eab

parser.add_argument("--aircraft", metavar="<filename>",
help="specifies the name of the aircraft to be modeled")
parser.add_argument("--script", metavar="<filename>",
help="specifies a script to run")

In addition, renaming the argument --planet has a number of inconvenience:

  1. The C++ code is using --planet so changing the argument only in the Python code would make the C++ and Python code inconsistent:

    jsbsim/src/JSBSim.cpp

    Lines 735 to 741 in 5be7eab

    } else if (keyword == "--planet") {
    if (n != string::npos) {
    PlanetName = SGPath::fromLocal8Bit(value.c_str());
    } else {
    gripe;
    exit(1);
    }

    cout << " --planet=<filename> specifies a planet definition file" << endl;
  2. The commit 853334f that introduced the argument --planet is now 2 years old meaning that it is part of the releases 1.2.0 and 1.2.1. Changing the argument name would break backward compatibility.

So I'd suggest not to approve this PR, unless a compelling reason to do so is given.

@T-Wainwright
Copy link
Contributor Author

T-Wainwright commented Jan 9, 2025

Happy to elaborate, I expect I'm running into this as someone coming at it from a pure python scripting perspective, so might have slipped through the testing cracks. But either way no way of running in python script mode currently works.

  1. pip install jsbsim
  2. JSBSim.py

You'll get the error:


Traceback (most recent call last):
  File "/Users/tom.wainwright/Documents/cases/DEMO/JSBSim/jsb_env/bin/JSBSim.py", line 184, in <module>
    if args.planetfile:
       ^^^^^^^^^^^^^^^

Alternative change is to change L184 to if args.planet: But either way the argparse name, and the reference to it need to match up. I wasn't aware there was convention in other bits of the code so happy to just change L184 too.

@bcoconni
Copy link
Member

bcoconni commented Jan 9, 2025

Alternative change is to change L184 to if args.planet: But either way the argparse name, and the reference to it need to match up. I wasn't aware there was convention in other bits of the code so happy to just change L184 too.

Thanks for further explanations. Good catch ! So yes, please replace L184 as you suggested.

Copy link

codecov bot commented Jan 9, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 24.96%. Comparing base (5be7eab) to head (df05f6a).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1215   +/-   ##
=======================================
  Coverage   24.96%   24.96%           
=======================================
  Files         170      170           
  Lines       19281    19281           
=======================================
  Hits         4814     4814           
  Misses      14467    14467           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bcoconni bcoconni merged commit a249c02 into JSBSim-Team:master Jan 9, 2025
30 checks passed
@bcoconni
Copy link
Member

bcoconni commented Jan 9, 2025

The PR has been merged.
Thanks for your contribution !

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.

3 participants