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

👌 IMPROVE: Allow copying empty lines #127

Merged
merged 2 commits into from
Jun 6, 2021

Conversation

amotl
Copy link
Contributor

@amotl amotl commented May 27, 2021

Dear Chris,

this is an attempt to resolve #84 reported by @chrisjsewell. Let me know what you think about it.

With kind regards,
Andreas.


Edit: As per discussion at #127 (comment) ff., this patch, now, both:

  • brings in the baseline feature (f18bd05)
  • and makes "copying empty lines" the default behavior (da08b04).

@welcome
Copy link

welcome bot commented May 27, 2021

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out EBP's Code of Conduct and our Contributing Guide, as this will greatly help the review process.

Welcome to the EBP community! 🎉

@chrisjsewell
Copy link
Member

Thanks I'll have a look soon

@amotl
Copy link
Contributor Author

amotl commented May 28, 2021

Hi @chrisjsewell and @choldgraf,

@sappelhoff suggested at #128 (review) (thanks!):

It's probably a better idea to stick with the #127 PR and make comments there about how it can be tweaked to make "copying empty lines" the default behavior. Otherwise, there will be two nearly identical PRs open which will confuse reviewers and will lead to diverging discussions that are easy to miss.

I will be very happy to add da08b04 to this PR instead of #128, in order to make copying empty lines the default, if both of you agree it would be the right choice.

At #84 (comment), @sappelhoff notes:

I would vote for making copying empty lines the default.

With kind regards,
Andreas.

@amotl
Copy link
Contributor Author

amotl commented May 31, 2021

Hi @chrisjsewell and @choldgraf,

do you also agree that we should make copying empty lines the default behavior? If so, I would add da08b04 on top of f18bd05 here in order to make that happen before a code review.

With kind regards,
Andreas.

@choldgraf
Copy link
Member

choldgraf commented May 31, 2021

Thanks for the ping - yep I agree we should make copying empty lines the default behavior. if there are two PRs doing very similar things it’ll be easier to just discuss in a single PR.

@amotl
Copy link
Contributor Author

amotl commented May 31, 2021

Dear Chris,

thanks for your response. I just added da08b04 to this patchset and closed #128.

With kind regards,
Andreas.

@amotl
Copy link
Contributor Author

amotl commented Jun 1, 2021

Can you have a look at this, @choldgraf or @chrisjsewell?

@choldgraf
Copy link
Member

choldgraf commented Jun 1, 2021

hey @amotl - thanks for this update. please be patient, we are all split across many projects and responsibilities :-)

@amotl
Copy link
Contributor Author

amotl commented Jun 2, 2021

Hi Chris,

Please be patient, we are all split across many projects and responsibilities :-)

Sure, same thing here. I didn't want to come off impatient, apologies!

With kind regards,
Andreas.

@choldgraf
Copy link
Member

This looks great to me - the implementation seems correct, and you've added docs and test cases. I'll give this a merge and we can spot-check updates if needed. Thanks for the improvement 🎉

@choldgraf choldgraf changed the title IMPROVE: Add feature for copying empty lines 👌 IMPROVE: Allow copying empty lines Jun 6, 2021
@choldgraf choldgraf merged commit 80fd656 into executablebooks:master Jun 6, 2021
@welcome
Copy link

welcome bot commented Jun 6, 2021

Congrats on your first merged pull request in this project! 🎉
congrats

Thank you for contributing, we are very proud of you! ❤️

@amotl amotl deleted the amo/copy-empty-lines branch June 9, 2021 15:19
@amotl
Copy link
Contributor Author

amotl commented Jun 9, 2021

Dear @choldgraf,

thanks a stack for merging this patch!

Now, hoping not to come over too impatient again, can I humbly ask one of the maintainers to run a new release cycle? Then, we would be able to integrate this feature back into our documentation (crate/crate-docs-theme#288).

With kind regards,
Andreas.

@choldgraf
Copy link
Member

choldgraf commented Jun 9, 2021

Maybe we can get #126 merged quickly and then release? If you have any comments on @sappelhoff's PR perhaps that can push things forward, but I think it is quite close

@sappelhoff
Copy link
Contributor

Maybe we can get #126 merged quickly and then release?

Would anything speak against including the two translation PRs #116 #120 in that release?

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.

Copy blanklines on matches(?)
4 participants