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

Dev #26

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

Dev #26

wants to merge 23 commits into from

Conversation

digikar99
Copy link

@digikar99 digikar99 commented May 23, 2020

I guess the commits mostly cover all the changes; those that do not seem well mentioned include -

  • travis now checks for both ecl and allegro
  • sionescu fork for travis is a maintained one, with more recent implementation versions; these updates in turn demanded a need for the addons - perhaps, this should be taken care of in the travis script
  • unix-opts-tests is renamed to unix-opts/tests; and (unix-opts/tests:run) runs the tests; there is some test refactoring as well; fiveam, local-nicknames, asdf:test-op are not supported on all implementations (clisp in particular); hence, cannot be used.

I am not sure about the versioning conventions, but the change on adding ambiguous-option condition might be significant enough for some people to demand a version bump.

xaxo05 and others added 23 commits February 8, 2019 21:07
…is another longer option of the form --option-long.
In Allegro CL, the symbol `sys:command-line-arguments' is a function
and must be called in order to correctly access command line arguments.
opts::print-opts can definitely still improve

libre-man#21
@digikar99
Copy link
Author

While I had attempted default, your implementation with the consideration of mutable args is probably better; as well as my implementation is incomplete missing out on a few aspects such as modifying the describe function.

@libre-man
Copy link
Owner

Hi thanks @digikar99 for the extensive fixes!

I will cherry pick/manually extract the features from it one by one if that is ok by you. The first one I've done was improving the testing in #27. In the next few days I'll extract the macro for the shorter definition of the options.

@digikar99
Copy link
Author

Yes that's perfectly okay; if anything, the way it has been done with the recent PR is much better than what I had attempted.

@digikar99
Copy link
Author

Looks like most of the things have been merged. Two of the things that seem to remain include ambiguous-option and expand-opts. Any opinions about them?

@libre-man
Copy link
Owner

I'm still planning on merging them, but I haven't had time for it just yet, also because it needs quite some work to split them up especially as the master now contains some of the functionality already of this PR. I'll probably have some time this weekend to get started on it again, so hopefully I'll be able to merge the expand-opts soon.

@digikar99
Copy link
Author

I'd be willing to close this PR - this looks pretty dirty anyways - and perhaps issue a separate PR explicitly for the expand-opts.

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.

4 participants