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

Feature/oops 4650/modify formatter logic #3

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

grzark
Copy link
Collaborator

@grzark grzark commented Jul 28, 2020

These updates focus mostly on indentation/line breaks when single line comments are present either "in line" or as a line before a block of code.

@grzark grzark requested review from numo16 and ajhall July 28, 2020 14:39
Copy link
Member

@ajhall ajhall left a comment

Choose a reason for hiding this comment

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

Can you add a summary to the PR description of what's included in these changes?

@@ -34,7 +34,7 @@ static ObfuscatingKeywordMapping()
Instance.Add("LEFT OUTER JOIN", "LEFT JOIN");
Instance.Add("RIGHT OUTER JOIN", "RIGHT JOIN");
Instance.Add("FULL OUTER JOIN", "FULL JOIN");
Instance.Add("INNER JOIN", "JOIN");
//Instance.Add("INNER JOIN", "JOIN");
Copy link
Member

Choose a reason for hiding this comment

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

Disabling this line stops the formatter from converting JOIN to INNER JOIN, right?

If that's how this works most of the entries in this file look ok, but does that mean it also converts VARCHAR to CHAR VARYING and INT to INTEGER?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I reverted this change as I found that these mappings are only utilized when the KeywordStandardization option of the formatter is set to true

@grzark grzark requested a review from ajhall July 28, 2020 18:03
Comment on lines 567 to 571
switch (contentElement.Name)
{
case SqlStructureConstants.ENAME_EXPRESSION_PARENS:
break;
}
Copy link
Member

Choose a reason for hiding this comment

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

Does this section do anything?

The outer switch statement is on contentElement.Parent.Name, and this inner one is on contentElement.Name.

The new case for SqlStructureConstants.ENAME_FUNCTION_PARENS catches something that would have otherwise fallen through to the default case, but the inner switch statement doesn't seem like it does anything.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I remember that. Probably an errant test artifact that I had breakpoints on..... Debugging this code is a bear

Copy link
Member

@ajhall ajhall Jul 28, 2020

Choose a reason for hiding this comment

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

Yeah, I looked through this code for the first time as part of this PR, and... wow, what a huge switch statement. Thanks for working on this 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed the switch case to nowhere and added a comment on why the empty case is present

Copy link
Member

@ajhall ajhall left a comment

Choose a reason for hiding this comment

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

Looks good!

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