Skip to content

Conversation

@sagi1623
Copy link
Contributor

As it can be seen in this comment, I realised making Parameters local was a mistake, as Named arguments is a thing for both C# and VbNet.
There are actually test cases in the Expressions file for both C# and VbNet, that I have glimpsed over because of the sheer amount of changes the PR introduced.

Sorry once more for the mistake.

@sagi1623
Copy link
Contributor Author

@olafurpg,

I have made a mistake in the previous PR! Sorry for that.
This one is here to revert part of the changes I have done there.
I promise, next time I will think twice before doing changes.

Copy link
Contributor

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻 Thank you so much for following up on this. I should have been more careful with the review, I'm not familiar enough with C# to know at the top of my head that it supports named arguments (although I recall it did, I ignored it in the PR).

@olafurpg olafurpg merged commit c83339c into sourcegraph:main Jul 14, 2023
@sagi1623 sagi1623 deleted the AraGimLeg/revert-making-params-local branch July 14, 2023 11:40
@sagi1623
Copy link
Contributor Author

It is completely my fault for missing the named argument usage.
The problem is that even though I enjoy working on this a lot, I usually don't have the time to do so during the day. And after a full workday around 10 PM in the evening it is easy for me to miss things. I promise I will be more careful in the future.

@olafurpg
Copy link
Contributor

@sagi1623 no worries at all!

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