Skip to content

Join sources on single destination #339

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

Merged
merged 3 commits into from
Nov 29, 2021
Merged

Conversation

AndreasArvidsson
Copy link
Member

No description provided.

Copy link
Member

@pokey pokey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tbh I find the impl a bit confusing, but I'm going to rework this code soon anyways so happy to leave it as long as it has decent test coverage. But would be good to add tests for the following:

  • A couple "before" examples
  • Another delimited scope type, eg "arg"

@AndreasArvidsson
Copy link
Member Author

The only thing I changed was to join the text on multiple sources into one destination
Nothing in this pr has changed behavior of before and after. We could probably add a couple of tests anyway.

@pokey
Copy link
Member

pokey commented Nov 29, 2021

The only thing I changed was to join the text on multiple sources into one destination Nothing in this pr has changed behavior of before and after. We could probably add a couple of tests anyway.

Yeah I mean adding tests where you bring multiple before. I could see that getting broken somehow where we end up adding two delimiters. And same adding tests where we bring multiple to a target that is an arg. Also prob useful "bring air and bat to arg cap"; I think currently your tests just do after and cursor

@AndreasArvidsson
Copy link
Member Author

I understand in completely agree. Tests added

Copy link
Member

@pokey pokey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great. I get the impression these extra tests might have found a bug 😊

b
c

const values = [e, a, b, c]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is actually pretty cool 😊

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great. I get the impression these extra tests might have found a bug 😊

Maybe

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is really cool!

@AndreasArvidsson AndreasArvidsson merged commit 1de4725 into main Nov 29, 2021
@AndreasArvidsson AndreasArvidsson deleted the bring_join_sources branch November 29, 2021 15:13
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.

2 participants