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

CLI Improvements #82

Merged
merged 9 commits into from Jun 1, 2019
Merged

CLI Improvements #82

merged 9 commits into from Jun 1, 2019

Conversation

newsch
Copy link
Member

@newsch newsch commented May 26, 2019

With the shiny new cli, you can:

  • continue to connect to a server
  • continue to load from a file or stdin
  • specify a height/width for the canvas, (which will also resize it after loading a file)
  • get an automagic git describe version
  • file an issue on github
  • get an introduction to the interface

I made the current version with argp, but that's GNU C and @labseven had some trouble compiling it on macOS. The options are:

TODO:

  • pick one of the above methods
  • validate arguments
  • add note about default canvas size
  • remove test print comments
  • rebase and squash before merge

Closes #62

@newsch newsch added the enhancement New feature or request label May 26, 2019
@newsch
Copy link
Member Author

newsch commented May 28, 2019

Right now argtable looks like the best option - cross-platform, offers a better interface than getopt, and would be easy to integrate (just a single .c and .h file).

@newsch
Copy link
Member Author

newsch commented May 28, 2019

As mentioned here, argp can also be included in the project through gnulib. This is my new favorite option.

@newsch
Copy link
Member Author

newsch commented May 28, 2019

Side note: libiberty is a pretty funny name.

@newsch
Copy link
Member Author

newsch commented May 28, 2019

As mentioned here, argp can also be included in the project through gnulib. This is my new favorite option.

Nevermind, it needs autoconf and would add 142 new files and 35k LOC.

@newsch newsch marked this pull request as ready for review May 28, 2019 03:56
Copy link
Collaborator

@MatthewBeaudouinLafon MatthewBeaudouinLafon left a comment

Choose a reason for hiding this comment

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

A few minor comments, but it looks good otherwise. Happy to approve once you've taken a look.

src/canvas_test.c Outdated Show resolved Hide resolved
src/frontend.c Outdated
*
* Return values are meant for an argp parser function.
*
* TODO: fill this out
Copy link
Collaborator

Choose a reason for hiding this comment

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

Either do this or make an issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

const char *program_description = "a collaborative ascii editor";
const char *program_doc =
"COLLASCII\n\n"
"\"The Future Editor of Yesterday, Tomorrow!\"\n\n"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be good to have a word here on starting a server (for someone who hasn't gone through extensive collascii training). Something like:

To start a collascii server, make and run server.out with make server.out && ./server.out

Copy link
Member Author

Choose a reason for hiding this comment

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

Would the README be a better place for that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That too. I'm thinking that someone who just downloaded collascii after skimming the Readme might do --help and like to see that info there.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added some changes the README and the CLI.

src/frontend.c Show resolved Hide resolved
src/frontend.c Outdated Show resolved Hide resolved
With the shiny new cli, you can:
- continue to connect to a server
- continue to load from a file or stdin
- specify a height/width for the canvas, (which will also resize it
  after loading a file)
- get an automagic `git describe` version
- file an issue on github
- get an introduction to the interface

In doing this, I refactored the state initialization into a function
and added an arguments_t struct that represents the state of the
arguments passed in.

I also made some network-related variables global - not sure if that
is the best way to do this.
@newsch
Copy link
Member Author

newsch commented May 31, 2019

Happy to approve once you've taken a look.

Rebased and ready!

@newsch newsch self-assigned this Jun 1, 2019
@MatthewBeaudouinLafon MatthewBeaudouinLafon merged commit b79ae49 into master Jun 1, 2019
@newsch newsch deleted the newsch/cli branch December 23, 2019 01:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add real cmdline flag parsing
2 participants