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

Exposing a DisableVariableSubstitution on the Parser object #1941

Merged
merged 5 commits into from
Mar 14, 2023

Conversation

Matteo-T
Copy link
Contributor

@Matteo-T Matteo-T commented Mar 13, 2023

Addressing issues #1936 and #1937.

With this change, I'm adding a new property on the Parser object DisableVariableSubstitution to allow the parsing to completely ignore elements in the T-SQL fragment that may look like Variables, thus allowing consumers of the Parser (e.g. SQL Server Powershell Invoke-Sqlcmd cmdlet) to micking the behavior of sqlcmd.exe -x which completely ignores such variables during parsing.

This property has a default value of false (for backward compatibility).

This PR also introduced the fix for #1937 is just to allow the Parser to be a little more forgiving and avoid throwing a NRE when the caller is not passing in any IVariableResolver, which most likely means "I do not need/care for any variable substitution, so don't bother".

While combining the 2 fixes, it made sense to change the default value for DisableVariableSubstitution to be true when the IVariableResolver is null (this is safe, since no existing code out there allowed it before... so no existing consumer should be surprised by this change).

This PR does not mean to address #1938, although it is somehow related to it.

@Matteo-T
Copy link
Contributor Author

Matteo-T commented Mar 13, 2023

@cheenamalhotra - could you advise on a couple of things? Like, is there a way to link the PR to the issues? Also, are there other things that need to happen before this PR can go in? Like updating labels, milestons, projects, etc? I suppose that whoever is triaging the issues is going to do that? I need to be educated... Thanks!


In reply to: 1465862486


In reply to: 1465862486


In reply to: 1465862486

@Charles-Gagnon
Copy link
Contributor

Charles-Gagnon commented Mar 13, 2023

For linking items to a PR the easiest way is to use one of the closing keywords : https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

Note that you have to put it in front of each issue to close - e.g.

Fixes https://github.com/microsoft/sqltoolsservice/issues/1936
Fixes https://github.com/microsoft/sqltoolsservice/issues/1937

In reply to: 1466382921


In reply to: 1466382921

@Matteo-T
Copy link
Contributor Author

I went with the UI (Development): it resembles ADO after all...


In reply to: 1466382921

@Matteo-T Matteo-T merged commit ffed831 into main Mar 14, 2023
@Matteo-T Matteo-T deleted the dev/matteot/AddingDisableVariableSubstitutionToParser branch March 14, 2023 05:04
Matteo-T added a commit that referenced this pull request Mar 14, 2023
* Cleaning up a few tests; fixing one

* Exposing DisableVariableSubstitution in Parser; fixing NRE; adding tests

* Added another test

* Addressing CR feedback; added tests
Matteo-T added a commit that referenced this pull request Mar 14, 2023
* Cleaning up a few tests; fixing one (#1940)

* Exposing a DisableVariableSubstitution on the Parser object (#1941)

* Cleaning up a few tests; fixing one

* Exposing DisableVariableSubstitution in Parser; fixing NRE; adding tests

* Added another test

* Addressing CR feedback; added tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants