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

Commons-cli based CLI args parser #983

Merged

Conversation

MarcinOrlowski
Copy link
Member

@MarcinOrlowski MarcinOrlowski commented Aug 31, 2021

This PR replaces old argument parsing with Apache's commons-cli based.

  • Completely rewritten command line argument parser:
    • All options have both short and long version now,
    • All long arguments require -- prefix i.e. --version,
    • All long arguments require single - as prefix i.e. -v,
    • -clearprefs is now --clear-prefs or -cp,
    • -clearprops option is remove (use --clear-prefs instead),
    • -geom is now --geometry,
    • -nosplash is now --no-splash or `-ns,
    • -sub is now --substitute or -s,
    • -testvector is now --test-vector or -tv.
    • removed -questa

NOTE: this is exact port of what we had in develop/ and I got a feeling certain options are no longer needed or whatever, so please do the check and let me know.

Tracing ticket #981

Fixes #981
Fixes #963

DEPENDS_ON #977

@MarcinOrlowski MarcinOrlowski mentioned this pull request Aug 31, 2021
5 tasks
@MarcinOrlowski MarcinOrlowski added this to the 3.7.0 milestone Aug 31, 2021
@github-actions github-actions bot added the dependent Blocked by Dependency Issuses bot label Aug 31, 2021
@MarcinOrlowski MarcinOrlowski marked this pull request as draft August 31, 2021 15:39
@MarcinOrlowski MarcinOrlowski self-assigned this Aug 31, 2021
@github-actions github-actions bot removed the dependent Blocked by Dependency Issuses bot label Sep 2, 2021
@github-actions
Copy link

github-actions bot commented Sep 2, 2021

🎉 Great news! Looks like all the dependencies have been resolved:

💡 To add or remove a dependency please update this issue/PR description.

Brought to you by Dependent Issues (:robot: ). Happy coding!

@R3dst0ne
Copy link
Contributor

R3dst0ne commented Sep 2, 2021

we should only add short forms for options where it makes sense and does not cause potential confusion. E.g., -h and -v for --help and --version are fine for me, but the others might be misleading.

I fully agree to this point. It is better to have no short version than a misleading one.

usage: Logisim-evolution [-b] [-e] [-f] [-g] [-h] [-j ] [-l ] [-m ] [-n] [-o ] [-p] [-r] [-s ] [-t ] [-v] [-w] [-x <yes/no>] [-z ]
-b, --test-circuit Open up a circ file and start the test bench...
-e, --empty Use empty template.
-f, --test-fpga Test implementations design ...
-g, --gates Sets "shaped" or "rectangular" gate style.
-h, --help Displays this argument summary help page.
-j, --test-circ-gen Open up the circ file and write a new one ...
-l, --load Load image file into RAM (works with -tty only).
-m, --geometry Sets geometry for main window:
-n, --no-splash Hides splash screen at startup.
-o, --locale Sets locale as given as argument.
-p, --plain Use standard Logisim template.
-r, --clear-prefs Clear application preferences at startup.
-s, --substitute Substitutes library lib1 with lib2.
Arguments: lib lib2.
-t, --tty Run without graphical interface. Argument:
TTY format.
-v, --version Display version number and exit
-w, --test-vector argTestVectorOption
-x, --accents <yes/no> Use accented characters or ASCII equivalents.
Arguments: "yes" or "no".
-z, --template Use file as template. Argument: file.

IMHO, only -h, -v, -e, -p make sense. I would add -t as short form for --template and -c, for --clear-prefs.
-g, -sand -lcan stay if you think they make sense (you could add -a for --accents).

@R3dst0ne
Copy link
Contributor

R3dst0ne commented Sep 2, 2021

As suggested in #963 and this comment:
ANSI and IEC might be better options than shaped and rectangular for the gate style option.

I think geometry is not self-explanatory. Maybe window would make more sense.

What about: (It could make sense to do those changes in a separate PR.)

  • -accents does not seem to work. (I propose to overhaul and rename it to -ascii.)

@R3dst0ne
Copy link
Contributor

R3dst0ne commented Sep 2, 2021

Both --help and --version exit with value 1.
They should exit with 0.

@MarcinOrlowski
Copy link
Member Author

I propose we tweak/add/remove arguments once ported in separate effort.

@MarcinOrlowski
Copy link
Member Author

I think geometry is not self-explanatory.
It's commonly used option for window geometry.

@MarcinOrlowski
Copy link
Member Author

MarcinOrlowski commented Sep 2, 2021

shape ANSI/IEC change

This needs also be separate effort. As we need to rename constants, update help string, localization etc. You can create separate ticket for this change though.

@MarcinOrlowski
Copy link
Member Author

Both --help and --version exit with value 1.
They should exit with 0.

Fixed.

CHANGES.md Outdated Show resolved Hide resolved
@MarcinOrlowski
Copy link
Member Author

Updated CHANGELOG. You'd need to remove this entry from 3.6.0 branch

@BFH-ktt1 BFH-ktt1 merged commit e661b0d into logisim-evolution:develop Sep 3, 2021
@MarcinOrlowski MarcinOrlowski deleted the commons-cli-arg-parse branch September 4, 2021 09:34
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.

None yet

4 participants