-
Notifications
You must be signed in to change notification settings - Fork 161
Add target to runtime benchmarks #2285
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left a couple of suggestions about the casing of the SQL keywords in the statements. Rather than comment on each one could you please go through capitalising the keywords. It's easier to read and also distinguishes the old queries from the new ones we've written.
I agree it's easier to read, but I'm not sure what do you mean by the distinction. I have not added any new queries apart from migrations in this PR. Do you suggest to automatically capitalize SQL in any query that we modify from now on? |
Yes I think we should capitalize SQL in queries we modify. Whay I mean by "distinction" is that the old queries are lower case so it's easy to see what is old and what is new - though this is subjective |
I don't think that we necessarily need to distinguish queries, there is not really any specific point where we could say that a query is "old" or "new"; this PR is not even primarily related to the job queue. But I agree that it's a reasonable drive-by refactoring, so I went ahead and capitalized the modified queries. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
So that we can run runtime benchmarks on ARM machines.