Skip to content

Conversation

@tobyWorland
Copy link
Contributor

Fulfills issue #1

@CLAassistant
Copy link

CLAassistant commented Jul 26, 2022

CLA assistant check
All committers have signed the CLA.

@tobyWorland tobyWorland marked this pull request as ready for review July 26, 2022 11:10
@Symbolics
Copy link
Contributor

Symbolics commented Jul 27, 2022

Looks good. Two things:

  1. Will you please make split-stems a keyword option in stem-and-leaf. This way the usage would be something like: (stem-and-leaf x :split-stems t). The keyword defaults should be nil
  2. Squash all commits with a single commit message like "implement back to back stem plots". See how to write a good commit message for some guidelines.

@Symbolics
Copy link
Contributor

I've just edited my comment. I had split brain syndrome :-) because initially we were discussing split-stems and this PR implements back-to-back. We really shouldn't mix &optional and &keys parameters, so back-to-back does need to be separate since it takes both x and y. split-stems we can signal with a keyword to stem-and-leaf.

Thanks for the contribution!

@tobyWorland tobyWorland force-pushed the feature/back-to-back branch from c822291 to 405c7e7 Compare July 27, 2022 14:13
@tobyWorland
Copy link
Contributor Author

Thanks for the review!

initially we were discussing split-stems and this PR implements back-to-back.

Ah, I think you're confusing me for @jg20019. I have not participated in the discussion for the other issue.

Squash all commits with a single commit message like "implement back to back stem plots".

Done

Will you please make split-stems a keyword option ...

Done, plus I added it to the docstring.

@Symbolics Symbolics merged commit 9a62112 into Lisp-Stat:master Jul 28, 2022
@Symbolics
Copy link
Contributor

Well done, thanks!

@tobyWorland tobyWorland deleted the feature/back-to-back branch July 28, 2022 17:33
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.

3 participants