armake2 icon indicating copy to clipboard operation
armake2 copied to clipboard

Restructure

Open BrettMayson opened this issue 6 years ago • 6 comments
trafficstars

This turns armake2 into a proper Rust library.

It is still missing a few small things from master but is ready for testing and any suggested tweaks

Notes signing is now a feature, meaning armake2 can be used without openssl.

BrettMayson avatar Jul 31 '19 13:07 BrettMayson

CI is failing build.

jonpas avatar Aug 04 '19 10:08 jonpas

The tests have not been modified yet, I mainly need people to test the restructure and suggest any changes they want to see since I rarely use armake2 personally

BrettMayson avatar Aug 04 '19 10:08 BrettMayson

It's very hard to comment on this PR as a whole since it contains so many changes in a single commit, so it would probably be easier to split this up into multiple PRs.

On some of the actual changes:

  • Making signing a feature is a great idea.
  • A lot of the general structure stuff (the grammar folder, etc.) is obviously fine.
  • Refactoring the error system a bit towards more idiomatic Rust is something I've been meaning to do, but it's difficult to judge seperately here. That probably deserves an entire PR/discussion for itself.
  • I dislike the move to clap; or rather the move away from docopt. I've yet to find a command line parser (in any language) that is as elegant and out-of-your-way as docopt is, and I think I would need to be convinced to abandon it.
  • Regarding the publication of the cmd_* functions: My intention with those regarding library functionality was to make them very thin wrappers around the library functions (for example, the cmd_rapify function litarally just uses Config::read and Config.write_rapified). They are basically glorified example functions. I think exposing them as part of the library is a bit unelegant, as it just seems like duplicate functionality. I would prefer to limit the library to a set of data structures with associated methods. What are the use cases that are served by exposing the command functions, and could those not be solved in a more elegant way?

KoffeinFlummi avatar Aug 05 '19 19:08 KoffeinFlummi

probably be easier to split this up into multiple PRs.

This changes so much that might not be possible. Separate issues could be used to discuss different parts of the restructure

BrettMayson avatar Aug 05 '19 22:08 BrettMayson

There really needs to be a confirm button for closing a PR

publication of the cmd_* functions

I'm fine with those not being public, they were basically all that was public in master so I left them public

BrettMayson avatar Aug 05 '19 22:08 BrettMayson

I believe they were originally marked as pub for some technical reason, although I could be misremembering. I got the impression that you were intending to make the commands more of a "proper" part of the library with the Command trait, but I guess I was mistaken and it was mostly for clap/the signature feature.

I'll try to play around with some of the changes myself this week; I might try to seperate some changes for overview.

KoffeinFlummi avatar Aug 07 '19 13:08 KoffeinFlummi