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

sp_BlitzQueryStore style, comments, and typo edits #3451

Closed
wants to merge 11 commits into from

Conversation

ReeceGoding
Copy link
Contributor

@ReeceGoding ReeceGoding commented Mar 4, 2024

I decided to clean up anything that bothered me in sp_BlitzQueryStore before beginning any serious work on it. These are largely style changes. Summary:

  1. Minor edits to some comments, mostly typo corrections and edits for consistency with surrounding comments.
  2. Removed some extra spaces, e.g. double spaces in printed text.
  3. Moved repeated calls to SERVERPROPERTY in to variables. Some of these originally were NVARCHAR and others were VARCHAR. I am not sure if there is a good reason behind this, so I have gone with my the docs. ENGINEEDITION is an exception to this and has been stored as an INT.
  4. Ruthlessly made the commenting syntax consistent. The most common form in the original was /*Foo*/ so I've applied that everywhere other than the multi-line block comments. Previously, it has --Foo, -- Foo, /*foo*/, /* Foo */, and
/*
I am a single sentence on a single line. This comment block has no other lines of text.
*/
  1. Added comments for some temp tables that had none.
  2. Documented the undocumented @Version parameters in the @Help text.
  3. Switched SELECT TOP N to SELECT TOP (N) for consistency with other uses of TOP.

I expect that only point 3 has even the slightest chance of breaking anything. Those variables are mostly used for Azure checks, but I don't have an Azure box to test them on.

@ReeceGoding
Copy link
Contributor Author

On the topic of tidying up comments, I unfortunately have no idea what "This looks for queries in the query stores that we picked up from an internal that have multiple plans in cache" is supposed to say. It looks like it has a missing word, but I don't know what.

@ReeceGoding ReeceGoding changed the title sp_BlitzQueryStore Style Edits sp_BlitzQueryStore style, comments, and typo edits Mar 15, 2024
@BrentOzar
Copy link
Member

I know you're not going to like hearing this, but:

  • Open an issue for each thing you want to change
  • Keep each pull request confined to fixing a single issue
  • Focus only on changes that merit a volunteer testing them for you (because someone else has to devote their time to reading your changes, understanding what they do, and testing them)

I totally understand that you put a lot of work into this pull request, but... just looking at the changed code makes my eyes water, and it's not fair to ask other folks to test this kind of thing for you.

@BrentOzar BrentOzar closed this Mar 29, 2024
@BrentOzar
Copy link
Member

I really do want to help you get changes made, but I just need you to abide by the readme & code contributions guide so that we can get code changes tested quickly and across the finish line. Sorry about that.

@ReeceGoding
Copy link
Contributor Author

@BrentOzar Don't worry. I've suspected for quite a while that I was trying to do too much in one PR. I've now broken this down in to new issues and submitted individual PRs for each issue that could be solved by chopping up the code that was in this PR. The diffs for the new PRs are a lot easier to digest than this massive one. Furthermore, a lot of them are just text/comment changes so there shouldn't be much testing effort needed. Sorry for the email spam that this must have caused you.

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.

None yet

2 participants