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

Restructure #50

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

Conversation

BrettMayson
Copy link
Contributor

@BrettMayson BrettMayson commented Jul 31, 2019

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.

@jonpas jonpas added the enhancement New feature or request label Aug 4, 2019
@jonpas jonpas requested a review from KoffeinFlummi August 4, 2019 10:32
@jonpas
Copy link
Collaborator

jonpas commented Aug 4, 2019

CI is failing build.

@BrettMayson
Copy link
Contributor Author

BrettMayson commented Aug 4, 2019

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

@KoffeinFlummi
Copy link
Owner

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?

@BrettMayson BrettMayson mentioned this pull request Aug 5, 2019
@BrettMayson
Copy link
Contributor Author

BrettMayson commented Aug 5, 2019

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 BrettMayson closed this Aug 5, 2019
@BrettMayson BrettMayson reopened this Aug 5, 2019
@BrettMayson
Copy link
Contributor Author

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

@KoffeinFlummi
Copy link
Owner

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.

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.

4 participants