Pass options via a json file instead of command line arguments #167

ZeyadTarekk · 2024-07-18T22:52:21Z

Fixes #164

First, the shim parses the arguments and creates a JSON file having those arguments
Then the shim passes the JSON file path to the cpp
The CPP code parses that JSON file and uses the arguments

arthaud

Nice, this is a good start.

Now we need to make it robust and follow our coding standard. See my comments.

Also: in shim.py, _get_command_options is now unused, can we remove it?

shim/shim.py

source/Main.cpp

shim/shim.py

source/Main.cpp

source/MarianaTrench.cpp

source/Options.cpp

arthaud

Nice, that looks very good!
Just a last change and we will be merging this.
Thanks for all your work.

source/Options.cpp

facebook-github-bot · 2024-07-24T12:30:20Z

@arthaud has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-07-24T20:07:56Z

@arthaud merged this pull request in 0ff695a.

ZeyadTarekk added 3 commits July 19, 2024 00:24

Parse the arguments at the shim and create a temp json file

fe7a8ed

Keep the created file

ba65ddc

Parse the arguments from the JSON file at cpp

46df932

facebook-github-bot added the cla signed label Jul 18, 2024

arthaud requested changes Jul 19, 2024

View reviewed changes

ZeyadTarekk added 5 commits July 19, 2024 16:00

Remove a new line in the beginning of function

9c47a3f

Set environment variables in a separate function

9eca26d

Remove the old options function

ca221e9

Return the filename without intermediate variable

d91949f

Return option json and write the file at the main

c72ddf6

arthaud reviewed Jul 22, 2024

View reviewed changes

shim/shim.py Outdated Show resolved Hide resolved

shim/shim.py Outdated Show resolved Hide resolved

shim/shim.py Outdated Show resolved Hide resolved

shim/shim.py Show resolved Hide resolved

shim/shim.py Outdated Show resolved Hide resolved

ZeyadTarekk added 9 commits July 22, 2024 21:53

Set the environemnt variables outside of the options method

9175f55

Remove debugging logs

0a74604

Edit some arguments type

313e36a

Handle extra_analysis_arguments in a different way

ef2359b

use boost program options to get the config file path

ea810d7

Remove parsing the json from the main

6cff675

Create static function to parse the arguments

f5168b6

Remove couts

e514945

parse the options from the json in the run method

f46ce38

ZeyadTarekk requested a review from arthaud July 23, 2024 00:17

ZeyadTarekk added 3 commits July 23, 2024 05:09

return unique_ptr instead of option ptr

28d1e36

Edit return type implementation

7275b9a

Handle unique_ptr

ef66403

arthaud requested changes Jul 23, 2024

View reviewed changes

source/Main.cpp Show resolved Hide resolved

source/MarianaTrench.cpp Outdated Show resolved Hide resolved

source/Options.cpp Outdated Show resolved Hide resolved

ZeyadTarekk added 3 commits July 23, 2024 15:59

Remove the intermediate variable

e8184ef

Parse the json file in the new constructor

dd52686

Remove old unused methods

9942cd9

ZeyadTarekk requested a review from arthaud July 23, 2024 17:32

arthaud requested changes Jul 23, 2024

View reviewed changes

source/Options.cpp Outdated Show resolved Hide resolved

source/Options.cpp Outdated Show resolved Hide resolved

source/Options.cpp Outdated Show resolved Hide resolved

source/Options.cpp Outdated Show resolved Hide resolved

ZeyadTarekk added 2 commits July 24, 2024 00:23

Add more validation methods

c00ac54

Validate the json values using JsonValidation

3401b48

ZeyadTarekk requested a review from arthaud July 23, 2024 21:25

anwesht requested changes Jul 23, 2024

View reviewed changes

source/Options.cpp Outdated Show resolved Hide resolved

ZeyadTarekk added 3 commits July 24, 2024 02:11

pass the jar paths as array of strings

0d87ac1

string_list json validation

8ab7b89

Parse jar paths as string_list

8fbcfbd

ZeyadTarekk requested a review from anwesht July 23, 2024 23:19

arthaud requested changes Jul 24, 2024

View reviewed changes

source/Options.cpp Outdated Show resolved Hide resolved

source/Options.cpp Outdated Show resolved Hide resolved

source/Options.cpp Outdated Show resolved Hide resolved

ZeyadTarekk added 4 commits July 24, 2024 14:34

remove string_list

5fb9c8a

retrurn system jar files as string

14f6b03

rename from_json_file method

269d189

accept std::filesystem::path

dfe8602

ZeyadTarekk requested a review from arthaud July 24, 2024 12:04

arthaud approved these changes Jul 24, 2024

View reviewed changes

facebook-github-bot closed this in 0ff695a Jul 24, 2024

facebook-github-bot added the Merged label Jul 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass options via a json file instead of command line arguments #167

Pass options via a json file instead of command line arguments #167

ZeyadTarekk commented Jul 18, 2024

arthaud left a comment

arthaud left a comment

facebook-github-bot commented Jul 24, 2024

facebook-github-bot commented Jul 24, 2024

Pass options via a json file instead of command line arguments #167

Pass options via a json file instead of command line arguments #167

Conversation

ZeyadTarekk commented Jul 18, 2024

arthaud left a comment

Choose a reason for hiding this comment

arthaud left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 24, 2024

facebook-github-bot commented Jul 24, 2024