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

Add prepend function and tests #45

Merged
merged 6 commits into from
Oct 20, 2021
Merged

Conversation

tanhakabir
Copy link
Contributor

Hey @cyjake 👋

I'm from the VS Code team and I was working on this issue: microsoft/vscode-remote-release#4592 when I realized I needed a prepend function from your library and rather than hacking prepend myself in the extension I wanted to contribute. I followed what you did in the append function and added a prepend function along with the same set of tests you had for append.

One part I wasn't completely certain about but I assumed the behavior of is when you have a config that is something like this:

Host test

Host test2
    Hostname google.com

and you prepend:

config.prepend({
      HostName: 'example.com',
      User: 'brian'
    })

I assumed the result should add the fields to the first config from the old config.

Host test
    Hostname example.com
    User brian

Host test2
    Hostname google.com

@tanhakabir
Copy link
Contributor Author

Added flag to enable prepending below Include directives as well. To support the scenarios described in microsoft/vscode-remote-release#4592 (comment)

@cyjake
Copy link
Owner

cyjake commented Oct 20, 2021

@tanhakabir Thanks for the pr! About the question in the pr description, in my first thought the expected result should be:

HostName example.com
User brian

Host test

Host test2
    HostName google.com

This result doesn't make any sense because it shadows Host test2 section, but I think we should keep prepend() simple. For example, if we prepend:

config.prepend({
  IdentityFile: '~/.ssh/id_rsa',
})

I'd expect the result to be:

# expected
IdentityFile ~/.ssh/id_rsa

Host test

Host test2
  HostName google.com

rather than:

Host test
  IdentityFile ~/.ssh/id_rsa

Host test2
  HostName google.com

@cyjake
Copy link
Owner

cyjake commented Oct 20, 2021

Oh, about the flag to change the prepend offset, can you change it to something like beforeFirstSection? For example, if I were to prepend a new section to following config:

IdentityFile ~/.ssh/id_rsa

Host test2
  HostName google.com

I can turn on the flag as well to get results like:

IdentityFile ~/.ssh/id_rsa

Host newlyAdded
  HostName newly.added

Host test2
  HostName google.com

If the flag is on but no section were found, just append the object to the end of the config.

@cyjake
Copy link
Owner

cyjake commented Oct 20, 2021

btw, I've updated master branch to see if we can trigger CI on pull request. Please rebase master before pushing branch.

@tanhakabir
Copy link
Contributor Author

@cyjake thanks for your feedback! Made the appropriate changes!

@tanhakabir
Copy link
Contributor Author

@cyjake I got a notification for a comment from you but I don't see anything. Just checking in to see if I missed anything?

@cyjake
Copy link
Owner

cyjake commented Oct 20, 2021

@tanhakabir yeah I made a comment without noticing I misunderstood the context.

@cyjake cyjake merged commit 1f33eb5 into cyjake:master Oct 20, 2021
@cyjake
Copy link
Owner

cyjake commented Oct 20, 2021

v4.1.0 released

@tanhakabir tanhakabir deleted the add-prepend branch October 21, 2021 16:29
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