Skip to content

Comments

C++/C#/Java/JavaScript/Python: Autoformat.#4748

Merged
MathiasVP merged 1 commit intogithub:mainfrom
aschackmull:autoformat-callchain
Dec 1, 2020
Merged

C++/C#/Java/JavaScript/Python: Autoformat.#4748
MathiasVP merged 1 commit intogithub:mainfrom
aschackmull:autoformat-callchain

Conversation

@aschackmull
Copy link
Contributor

@aschackmull aschackmull commented Nov 30, 2020

Experimental autoformatter change.

QL changes to go with the updated autoformatter: Call chains with a short initial qualifier skip the first linebreak.

@aschackmull aschackmull added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Nov 30, 2020
@aschackmull aschackmull marked this pull request as ready for review November 30, 2020 14:04
@aschackmull aschackmull requested review from a team as code owners November 30, 2020 14:04
Copy link
Contributor

@jbj jbj left a comment

Choose a reason for hiding this comment

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

C++ LGTM. I have one question, but it's not blocking for this PR.

Comment on lines -135 to +136
this
.hasQualifiedName("std", ["set", "unordered_set"],
["lower_bound", "upper_bound", "equal_range"])
this.hasQualifiedName("std", ["set", "unordered_set"],
["lower_bound", "upper_bound", "equal_range"])
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surprised by this change. I would have expected the second line to be indented two levels more. If hasQualifiedName has a result, could we then get

    this.hasQualifiedName("std", ["set", "unordered_set"],
      ["lower_bound", "upper_bound", "equal_range"])
        .hasSomeProperty()

That doesn't seem right. But perhaps it's not an issue in practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's actually a reasonable expectation, which I think makes perfect sense. Currently the indentation block isn't started until after printing the call, but it would make more sense to start it before, which would have this effect. It does require some refactoring of the autoformatter code, so I'll make a note of this and leave it for future work if that's ok.

Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

C# 👍

Copy link
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

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

JS 👍

@MathiasVP MathiasVP merged commit df29a16 into github:main Dec 1, 2020
@aschackmull aschackmull deleted the autoformat-callchain branch December 1, 2020 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C# C++ depends on internal PR This PR should only be merged in sync with an internal Semmle PR Java JS Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants