Skip to content

Conversation

eastwood
Copy link
Contributor

@eastwood eastwood commented Mar 20, 2023

Developer notes:

  • Bug is around some presumptious FP composition which doesn't consider errors thrown from outside environment.
  • Have filtered out issues where the config resolution is erroneous.
  • Would like to write tests, but the particular code path is not accessible by tests. The singleton appears to be making it difficult and debugging tests hasn't been entirely easy with the singletons.
  • Will have a go at injecting the config file dependency so that we test the scenario

@eastwood
Copy link
Contributor Author

@microsoft-github-policy-service agree

@eastwood eastwood marked this pull request as ready for review March 28, 2023 06:44
@alexr00 alexr00 added this to the April 2023 milestone Mar 28, 2023
Copy link
Member

@alexr00 alexr00 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I will merge after the March release.

@eastwood
Copy link
Contributor Author

You're welcome :) I performed some QA on this locally, but had a bit of difficulty hooking into the tests in protocol.test.ts due to the singletons. Would you be able to confirm that with a malformed .ssh/config file such as:

Host: 

The error logs are what you expect?

This is my first contribution to a vscode extension, so hoping that I used the idiomatic logging mechanism. Criticism is appreciated :)

@alexr00
Copy link
Member

alexr00 commented Mar 28, 2023

For this extension, the Logger that you used is the best way to do it

@alexr00 alexr00 enabled auto-merge (squash) April 11, 2023 10:51
@alexr00 alexr00 merged commit e54342f into microsoft:main Apr 11, 2023
@eastwood eastwood deleted the ssh-config-errors branch April 19, 2023 06:53
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