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

Spaces inserted after commas in command text #539

Closed
mcNux opened this issue Nov 20, 2020 · 3 comments · Fixed by #549
Closed

Spaces inserted after commas in command text #539

mcNux opened this issue Nov 20, 2020 · 3 comments · Fixed by #549

Comments

@mcNux
Copy link

mcNux commented Nov 20, 2020

Found that (even without a SqlFormatter configured) the SQL as it appears in MiniProfiler differs from the original command text; extra spaces are found after commas. The spaces are inserted here.

This is a problem for our team as we use MiniProfiler's output to diagnose poorly performing queries, and need to ensure same query plan is used (and thus exact same SQL) when we execute in SSMS.

As a workaround we're running the same regex on the SQL before it is executed.

@Turnerj
Copy link
Contributor

Turnerj commented Feb 22, 2021

While I do see that code that adds the comma, just curious, do you set any SQL formatter yourself on the options or just use the default (which seems to be the InlineFormatter)?

Digging through how the code works and what it calls, it looks like it should allow overriding of that via setting your own SQL formatter however that code, as seen here, also calls GetReadableCommand creating those spaces again.

What I'm thinking is removing the call to GetReadableCommand for the SQL formatters and make it an option in their constructors instead. That way you can control whether you want that additional spacing or not.

@mcNux
Copy link
Author

mcNux commented Feb 22, 2021

Yes, we do use own SQL formatter.

I think it would be ideal if no extra formatting (I'm considering adding spaces as formatting) occurred if custom formatter is applied but yeah making it configurable sounds good.

I shouldn't concern yourself too much though; probably not a problem for anyone else :).

@Turnerj
Copy link
Contributor

Turnerj commented Feb 23, 2021

Nah, I get what you mean - having the exact same query would be ideal even if the differences are only spaces. There is actually another issue just recently which has a problem with null values in queries appearing as empty strings instead - while still a valid query, it isn't the same query.

NickCraver pushed a commit that referenced this issue Jul 3, 2021
Fixes #539 

Default is enabled, like the existing GetReadableCommand logic.
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 a pull request may close this issue.

2 participants