You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "imply-cheddar (via GitHub)" <gi...@apache.org> on 2023/03/10 03:16:05 UTC
[GitHub] [druid] imply-cheddar opened a new pull request, #13916: Document our conventions for writing messages
imply-cheddar opened a new pull request, #13916:
URL: https://github.com/apache/druid/pull/13916
This adds some documentation for our message-writing conventions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] imply-cheddar commented on a diff in pull request #13916: Document our conventions for writing messages
Posted by "imply-cheddar (via GitHub)" <gi...@apache.org>.
imply-cheddar commented on code in PR #13916:
URL: https://github.com/apache/druid/pull/13916#discussion_r1133376919
##########
dev/style-conventions.md:
##########
@@ -0,0 +1,51 @@
+---
+id: style-conventions
+title: "Druid Style Guide"
+sidebar_label: "Druid Style Guide"
+---
+
+<!--
+ ~ Licensed to the Apache Software Foundation (ASF) under one
+ ~ or more contributor license agreements. See the NOTICE file
+ ~ distributed with this work for additional information
+ ~ regarding copyright ownership. The ASF licenses this file
+ ~ to you under the Apache License, Version 2.0 (the
+ ~ "License"); you may not use this file except in compliance
+ ~ with the License. You may obtain a copy of the License at
+ ~
+ ~ http://www.apache.org/licenses/LICENSE-2.0
+ ~
+ ~ Unless required by applicable law or agreed to in writing,
+ ~ software distributed under the License is distributed on an
+ ~ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ ~ KIND, either express or implied. See the License for the
+ ~ specific language governing permissions and limitations
+ ~ under the License.
+ -->
+
+This document attempts to describe the style that Druid code is expected to follow.
+
+A large amount of the style conventions are handled through IDE configuration and automated checkstyle rules.
+
+- For Intellij you can import our code style settings xml: [`druid_intellij_formatting.xml`](
+ https://github.com/apache/druid/raw/master/dev/druid_intellij_formatting.xml).
+- For Eclipse you can import our code style settings xml: [`eclipse_formatting.xml`](
+ https://github.com/apache/druid/raw/master/dev/eclipse_formatting.xml).
+
+While this page might discuss conventions that are also enforced via said mechanisms, the primary intent is to
+discuss style-related convention that cannot be (or are extremely difficult to be) enforced through such automated
+mechanisms.
+
+## Message Formatting (Logs and Exceptions)
+
+The way that log and exception messages get formatted is an important part of a project. Specifically, it is
+important that there is consistency in formatting such that someone can easily identify and interpret messages.
+This consistency applies to both log *and* exception messages.
+
+1. Messages should have something interpolated into them. Generally speaking, if the time is being taken to generate a message, it is usually valuable to interpolate something from the context into that message. There are exceptions to this, but all messages should start with the assumption that something should be interpolated and try to figure out what that is.
+ * Messages INFO level or above (this includes all Exceptions) cannot leak secrets or the content of data. When choosing what to interpolate, it is important to make sure that what is being added is not going to leak secrets or the contents of data. For example, a query with a malformed filter should provide an indication that the filter was malformed and which filter it is, but it cannot include the values being filtered for as that risks leaking data.
Review Comment:
Avoid for new message and fix up in older ones as we notice them. I included it here to try to make sure that it was called out as an important responsibility when interpolating things.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] cheddar merged pull request #13916: Document our conventions for writing messages
Posted by "cheddar (via GitHub)" <gi...@apache.org>.
cheddar merged PR #13916:
URL: https://github.com/apache/druid/pull/13916
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a diff in pull request #13916: Document our conventions for writing messages
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #13916:
URL: https://github.com/apache/druid/pull/13916#discussion_r1132061866
##########
dev/style-conventions.md:
##########
@@ -0,0 +1,51 @@
+---
+id: style-conventions
+title: "Druid Style Guide"
+sidebar_label: "Druid Style Guide"
+---
+
+<!--
+ ~ Licensed to the Apache Software Foundation (ASF) under one
+ ~ or more contributor license agreements. See the NOTICE file
+ ~ distributed with this work for additional information
+ ~ regarding copyright ownership. The ASF licenses this file
+ ~ to you under the Apache License, Version 2.0 (the
+ ~ "License"); you may not use this file except in compliance
+ ~ with the License. You may obtain a copy of the License at
+ ~
+ ~ http://www.apache.org/licenses/LICENSE-2.0
+ ~
+ ~ Unless required by applicable law or agreed to in writing,
+ ~ software distributed under the License is distributed on an
+ ~ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ ~ KIND, either express or implied. See the License for the
+ ~ specific language governing permissions and limitations
+ ~ under the License.
+ -->
+
+This document attempts to describe the style that Druid code is expected to follow.
+
+A large amount of the style conventions are handled through IDE configuration and automated checkstyle rules.
+
+- For Intellij you can import our code style settings xml: [`druid_intellij_formatting.xml`](
+ https://github.com/apache/druid/raw/master/dev/druid_intellij_formatting.xml).
+- For Eclipse you can import our code style settings xml: [`eclipse_formatting.xml`](
+ https://github.com/apache/druid/raw/master/dev/eclipse_formatting.xml).
+
+While this page might discuss conventions that are also enforced via said mechanisms, the primary intent is to
+discuss style-related convention that cannot be (or are extremely difficult to be) enforced through such automated
+mechanisms.
+
+## Message Formatting (Logs and Exceptions)
+
+The way that log and exception messages get formatted is an important part of a project. Specifically, it is
+important that there is consistency in formatting such that someone can easily identify and interpret messages.
+This consistency applies to both log *and* exception messages.
+
+1. Messages should have something interpolated into them. Generally speaking, if the time is being taken to generate a message, it is usually valuable to interpolate something from the context into that message. There are exceptions to this, but all messages should start with the assumption that something should be interpolated and try to figure out what that is.
+ * Messages INFO level or above (this includes all Exceptions) cannot leak secrets or the content of data. When choosing what to interpolate, it is important to make sure that what is being added is not going to leak secrets or the contents of data. For example, a query with a malformed filter should provide an indication that the filter was malformed and which filter it is, but it cannot include the values being filtered for as that risks leaking data.
Review Comment:
I think we have a bunch of cases where this isn't followed, do you think we should be embarking on a project of going through things and changing them? Or is it more something that we should try to avoid for new messages, and fix up in older ones as we notice them?
##########
dev/style-conventions.md:
##########
@@ -0,0 +1,51 @@
+---
+id: style-conventions
+title: "Druid Style Guide"
+sidebar_label: "Druid Style Guide"
+---
+
+<!--
+ ~ Licensed to the Apache Software Foundation (ASF) under one
+ ~ or more contributor license agreements. See the NOTICE file
+ ~ distributed with this work for additional information
+ ~ regarding copyright ownership. The ASF licenses this file
+ ~ to you under the Apache License, Version 2.0 (the
+ ~ "License"); you may not use this file except in compliance
+ ~ with the License. You may obtain a copy of the License at
+ ~
+ ~ http://www.apache.org/licenses/LICENSE-2.0
+ ~
+ ~ Unless required by applicable law or agreed to in writing,
+ ~ software distributed under the License is distributed on an
+ ~ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ ~ KIND, either express or implied. See the License for the
+ ~ specific language governing permissions and limitations
+ ~ under the License.
+ -->
+
+This document attempts to describe the style that Druid code is expected to follow.
+
+A large amount of the style conventions are handled through IDE configuration and automated checkstyle rules.
+
+- For Intellij you can import our code style settings xml: [`druid_intellij_formatting.xml`](
+ https://github.com/apache/druid/raw/master/dev/druid_intellij_formatting.xml).
+- For Eclipse you can import our code style settings xml: [`eclipse_formatting.xml`](
+ https://github.com/apache/druid/raw/master/dev/eclipse_formatting.xml).
+
+While this page might discuss conventions that are also enforced via said mechanisms, the primary intent is to
+discuss style-related convention that cannot be (or are extremely difficult to be) enforced through such automated
+mechanisms.
+
+## Message Formatting (Logs and Exceptions)
+
+The way that log and exception messages get formatted is an important part of a project. Specifically, it is
+important that there is consistency in formatting such that someone can easily identify and interpret messages.
+This consistency applies to both log *and* exception messages.
+
+1. Messages should have something interpolated into them. Generally speaking, if the time is being taken to generate a message, it is usually valuable to interpolate something from the context into that message. There are exceptions to this, but all messages should start with the assumption that something should be interpolated and try to figure out what that is.
+ * Messages INFO level or above (this includes all Exceptions) cannot leak secrets or the content of data. When choosing what to interpolate, it is important to make sure that what is being added is not going to leak secrets or the contents of data. For example, a query with a malformed filter should provide an indication that the filter was malformed and which filter it is, but it cannot include the values being filtered for as that risks leaking data.
+2. Interpolated values should always be encased in a `[]` and come after a noun that describes what is being interpolated. This is to ensure that enough context on what is happening exists and to clearly demark that an interpolation has occurred. Additionally, this identifies the start and end of the interpolation, which is important because messages that attempt to mimic natural prose that also include interpolation can sometimes mask glaring problems (like the inclusion of a space).
+ * Bad: `log.info("%s %s cannot handle %s", "null", "is not null", "INTEGER")` -> `"null is not null cannot handle INTEGER"`
+ * Better, but still not wonderful: `log.info("column[%s] filter[%s] cannot handle type[%s]", "null", "is not null", "INTEGER")` -> `"column[null] filter[is not null] cannot handle type[INTEGER]"`
Review Comment:
Looking at this, it reminds me one of my personal peeves is when messages are not in sentence case. If you agree then I would enjoy seeing something about that in the list.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] imply-cheddar commented on a diff in pull request #13916: Document our conventions for writing messages
Posted by "imply-cheddar (via GitHub)" <gi...@apache.org>.
imply-cheddar commented on code in PR #13916:
URL: https://github.com/apache/druid/pull/13916#discussion_r1133377469
##########
dev/style-conventions.md:
##########
@@ -0,0 +1,51 @@
+---
+id: style-conventions
+title: "Druid Style Guide"
+sidebar_label: "Druid Style Guide"
+---
+
+<!--
+ ~ Licensed to the Apache Software Foundation (ASF) under one
+ ~ or more contributor license agreements. See the NOTICE file
+ ~ distributed with this work for additional information
+ ~ regarding copyright ownership. The ASF licenses this file
+ ~ to you under the Apache License, Version 2.0 (the
+ ~ "License"); you may not use this file except in compliance
+ ~ with the License. You may obtain a copy of the License at
+ ~
+ ~ http://www.apache.org/licenses/LICENSE-2.0
+ ~
+ ~ Unless required by applicable law or agreed to in writing,
+ ~ software distributed under the License is distributed on an
+ ~ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ ~ KIND, either express or implied. See the License for the
+ ~ specific language governing permissions and limitations
+ ~ under the License.
+ -->
+
+This document attempts to describe the style that Druid code is expected to follow.
+
+A large amount of the style conventions are handled through IDE configuration and automated checkstyle rules.
+
+- For Intellij you can import our code style settings xml: [`druid_intellij_formatting.xml`](
+ https://github.com/apache/druid/raw/master/dev/druid_intellij_formatting.xml).
+- For Eclipse you can import our code style settings xml: [`eclipse_formatting.xml`](
+ https://github.com/apache/druid/raw/master/dev/eclipse_formatting.xml).
+
+While this page might discuss conventions that are also enforced via said mechanisms, the primary intent is to
+discuss style-related convention that cannot be (or are extremely difficult to be) enforced through such automated
+mechanisms.
+
+## Message Formatting (Logs and Exceptions)
+
+The way that log and exception messages get formatted is an important part of a project. Specifically, it is
+important that there is consistency in formatting such that someone can easily identify and interpret messages.
+This consistency applies to both log *and* exception messages.
+
+1. Messages should have something interpolated into them. Generally speaking, if the time is being taken to generate a message, it is usually valuable to interpolate something from the context into that message. There are exceptions to this, but all messages should start with the assumption that something should be interpolated and try to figure out what that is.
+ * Messages INFO level or above (this includes all Exceptions) cannot leak secrets or the content of data. When choosing what to interpolate, it is important to make sure that what is being added is not going to leak secrets or the contents of data. For example, a query with a malformed filter should provide an indication that the filter was malformed and which filter it is, but it cannot include the values being filtered for as that risks leaking data.
+2. Interpolated values should always be encased in a `[]` and come after a noun that describes what is being interpolated. This is to ensure that enough context on what is happening exists and to clearly demark that an interpolation has occurred. Additionally, this identifies the start and end of the interpolation, which is important because messages that attempt to mimic natural prose that also include interpolation can sometimes mask glaring problems (like the inclusion of a space).
+ * Bad: `log.info("%s %s cannot handle %s", "null", "is not null", "INTEGER")` -> `"null is not null cannot handle INTEGER"`
+ * Better, but still not wonderful: `log.info("column[%s] filter[%s] cannot handle type[%s]", "null", "is not null", "INTEGER")` -> `"column[null] filter[is not null] cannot handle type[INTEGER]"`
Review Comment:
I can add the thing on sentence-case.
The space comment has made me realize something else that goes along with the sentence-case that I will add to try to address why I tend to not add spaces. Once that's there, we can perhaps have the conversation on whether it makes sense or not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] imply-cheddar commented on a diff in pull request #13916: Document our conventions for writing messages
Posted by "imply-cheddar (via GitHub)" <gi...@apache.org>.
imply-cheddar commented on code in PR #13916:
URL: https://github.com/apache/druid/pull/13916#discussion_r1133377629
##########
dev/style-conventions.md:
##########
@@ -0,0 +1,51 @@
+---
+id: style-conventions
+title: "Druid Style Guide"
+sidebar_label: "Druid Style Guide"
+---
+
+<!--
+ ~ Licensed to the Apache Software Foundation (ASF) under one
+ ~ or more contributor license agreements. See the NOTICE file
+ ~ distributed with this work for additional information
+ ~ regarding copyright ownership. The ASF licenses this file
+ ~ to you under the Apache License, Version 2.0 (the
+ ~ "License"); you may not use this file except in compliance
+ ~ with the License. You may obtain a copy of the License at
+ ~
+ ~ http://www.apache.org/licenses/LICENSE-2.0
+ ~
+ ~ Unless required by applicable law or agreed to in writing,
+ ~ software distributed under the License is distributed on an
+ ~ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ ~ KIND, either express or implied. See the License for the
+ ~ specific language governing permissions and limitations
+ ~ under the License.
+ -->
+
+This document attempts to describe the style that Druid code is expected to follow.
+
+A large amount of the style conventions are handled through IDE configuration and automated checkstyle rules.
+
+- For Intellij you can import our code style settings xml: [`druid_intellij_formatting.xml`](
+ https://github.com/apache/druid/raw/master/dev/druid_intellij_formatting.xml).
+- For Eclipse you can import our code style settings xml: [`eclipse_formatting.xml`](
+ https://github.com/apache/druid/raw/master/dev/eclipse_formatting.xml).
+
+While this page might discuss conventions that are also enforced via said mechanisms, the primary intent is to
+discuss style-related convention that cannot be (or are extremely difficult to be) enforced through such automated
+mechanisms.
+
+## Message Formatting (Logs and Exceptions)
+
+The way that log and exception messages get formatted is an important part of a project. Specifically, it is
+important that there is consistency in formatting such that someone can easily identify and interpret messages.
+This consistency applies to both log *and* exception messages.
+
+1. Messages should have something interpolated into them. Generally speaking, if the time is being taken to generate a message, it is usually valuable to interpolate something from the context into that message. There are exceptions to this, but all messages should start with the assumption that something should be interpolated and try to figure out what that is.
+ * Messages INFO level or above (this includes all Exceptions) cannot leak secrets or the content of data. When choosing what to interpolate, it is important to make sure that what is being added is not going to leak secrets or the contents of data. For example, a query with a malformed filter should provide an indication that the filter was malformed and which filter it is, but it cannot include the values being filtered for as that risks leaking data.
+2. Interpolated values should always be encased in a `[]` and come after a noun that describes what is being interpolated. This is to ensure that enough context on what is happening exists and to clearly demark that an interpolation has occurred. Additionally, this identifies the start and end of the interpolation, which is important because messages that attempt to mimic natural prose that also include interpolation can sometimes mask glaring problems (like the inclusion of a space).
+ * Bad: `log.info("%s %s cannot handle %s", "null", "is not null", "INTEGER")` -> `"null is not null cannot handle INTEGER"`
+ * Better, but still not wonderful: `log.info("column[%s] filter[%s] cannot handle type[%s]", "null", "is not null", "INTEGER")` -> `"column[null] filter[is not null] cannot handle type[INTEGER]"`
+ * Good: `log.info("Filter[%s] on column[%s] cannot be applied to type[%s]", "is not null", "null", "INTEGER")` -> `"Filter[is not null] on column[null] cannot be applied to type[INTEGER]"`
+ * But, if I interpolate an array, I get double `[]`. Yes, that is true, that is okay. Said another way, if a log message contains `[[]]` that is indicative of interpolating an array-like structure.
Review Comment:
Yeah... that was intended as a quoted response of what someone might think when reading this. I agree it's weird, I'll adjust.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on pull request #13916: Document our conventions for writing messages
Posted by "abhishekagarwal87 (via GitHub)" <gi...@apache.org>.
abhishekagarwal87 commented on PR #13916:
URL: https://github.com/apache/druid/pull/13916#issuecomment-1463311247
LGTM.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekrb19 commented on a diff in pull request #13916: Document our conventions for writing messages
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on code in PR #13916:
URL: https://github.com/apache/druid/pull/13916#discussion_r1131996525
##########
dev/style-conventions.md:
##########
@@ -0,0 +1,51 @@
+---
+id: style-conventions
+title: "Druid Style Guide"
+sidebar_label: "Druid Style Guide"
+---
+
+<!--
+ ~ Licensed to the Apache Software Foundation (ASF) under one
+ ~ or more contributor license agreements. See the NOTICE file
+ ~ distributed with this work for additional information
+ ~ regarding copyright ownership. The ASF licenses this file
+ ~ to you under the Apache License, Version 2.0 (the
+ ~ "License"); you may not use this file except in compliance
+ ~ with the License. You may obtain a copy of the License at
+ ~
+ ~ http://www.apache.org/licenses/LICENSE-2.0
+ ~
+ ~ Unless required by applicable law or agreed to in writing,
+ ~ software distributed under the License is distributed on an
+ ~ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ ~ KIND, either express or implied. See the License for the
+ ~ specific language governing permissions and limitations
+ ~ under the License.
+ -->
+
+This document attempts to describe the style that Druid code is expected to follow.
+
+A large amount of the style conventions are handled through IDE configuration and automated checkstyle rules.
+
+- For Intellij you can import our code style settings xml: [`druid_intellij_formatting.xml`](
+ https://github.com/apache/druid/raw/master/dev/druid_intellij_formatting.xml).
+- For Eclipse you can import our code style settings xml: [`eclipse_formatting.xml`](
+ https://github.com/apache/druid/raw/master/dev/eclipse_formatting.xml).
+
+While this page might discuss conventions that are also enforced via said mechanisms, the primary intent is to
+discuss style-related convention that cannot be (or are extremely difficult to be) enforced through such automated
+mechanisms.
+
+## Message Formatting (Logs and Exceptions)
+
+The way that log and exception messages get formatted is an important part of a project. Specifically, it is
+important that there is consistency in formatting such that someone can easily identify and interpret messages.
+This consistency applies to both log *and* exception messages.
+
+1. Messages should have something interpolated into them. Generally speaking, if the time is being taken to generate a message, it is usually valuable to interpolate something from the context into that message. There are exceptions to this, but all messages should start with the assumption that something should be interpolated and try to figure out what that is.
+ * Messages INFO level or above (this includes all Exceptions) cannot leak secrets or the content of data. When choosing what to interpolate, it is important to make sure that what is being added is not going to leak secrets or the contents of data. For example, a query with a malformed filter should provide an indication that the filter was malformed and which filter it is, but it cannot include the values being filtered for as that risks leaking data.
+2. Interpolated values should always be encased in a `[]` and come after a noun that describes what is being interpolated. This is to ensure that enough context on what is happening exists and to clearly demark that an interpolation has occurred. Additionally, this identifies the start and end of the interpolation, which is important because messages that attempt to mimic natural prose that also include interpolation can sometimes mask glaring problems (like the inclusion of a space).
+ * Bad: `log.info("%s %s cannot handle %s", "null", "is not null", "INTEGER")` -> `"null is not null cannot handle INTEGER"`
+ * Better, but still not wonderful: `log.info("column[%s] filter[%s] cannot handle type[%s]", "null", "is not null", "INTEGER")` -> `"column[null] filter[is not null] cannot handle type[INTEGER]"`
+ * Good: `log.info("Filter[%s] on column[%s] cannot be applied to type[%s]`, "is not null", "null", "INTEGER")` -> `"Filter[is not null] on column[null] cannot be applied to type[INTEGER]"`
Review Comment:
There's an additional backtick in this line causing rich diff to not render correctly:
```suggestion
* Good: `log.info("Filter[%s] on column[%s] cannot be applied to type[%s], "is not null", "null", "INTEGER")` -> `"Filter[is not null] on column[null] cannot be applied to type[INTEGER]"`
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] imply-cheddar commented on a diff in pull request #13916: Document our conventions for writing messages
Posted by "imply-cheddar (via GitHub)" <gi...@apache.org>.
imply-cheddar commented on code in PR #13916:
URL: https://github.com/apache/druid/pull/13916#discussion_r1132059931
##########
dev/style-conventions.md:
##########
@@ -0,0 +1,51 @@
+---
+id: style-conventions
+title: "Druid Style Guide"
+sidebar_label: "Druid Style Guide"
+---
+
+<!--
+ ~ Licensed to the Apache Software Foundation (ASF) under one
+ ~ or more contributor license agreements. See the NOTICE file
+ ~ distributed with this work for additional information
+ ~ regarding copyright ownership. The ASF licenses this file
+ ~ to you under the Apache License, Version 2.0 (the
+ ~ "License"); you may not use this file except in compliance
+ ~ with the License. You may obtain a copy of the License at
+ ~
+ ~ http://www.apache.org/licenses/LICENSE-2.0
+ ~
+ ~ Unless required by applicable law or agreed to in writing,
+ ~ software distributed under the License is distributed on an
+ ~ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ ~ KIND, either express or implied. See the License for the
+ ~ specific language governing permissions and limitations
+ ~ under the License.
+ -->
+
+This document attempts to describe the style that Druid code is expected to follow.
+
+A large amount of the style conventions are handled through IDE configuration and automated checkstyle rules.
+
+- For Intellij you can import our code style settings xml: [`druid_intellij_formatting.xml`](
+ https://github.com/apache/druid/raw/master/dev/druid_intellij_formatting.xml).
+- For Eclipse you can import our code style settings xml: [`eclipse_formatting.xml`](
+ https://github.com/apache/druid/raw/master/dev/eclipse_formatting.xml).
+
+While this page might discuss conventions that are also enforced via said mechanisms, the primary intent is to
+discuss style-related convention that cannot be (or are extremely difficult to be) enforced through such automated
+mechanisms.
+
+## Message Formatting (Logs and Exceptions)
+
+The way that log and exception messages get formatted is an important part of a project. Specifically, it is
+important that there is consistency in formatting such that someone can easily identify and interpret messages.
+This consistency applies to both log *and* exception messages.
+
+1. Messages should have something interpolated into them. Generally speaking, if the time is being taken to generate a message, it is usually valuable to interpolate something from the context into that message. There are exceptions to this, but all messages should start with the assumption that something should be interpolated and try to figure out what that is.
+ * Messages INFO level or above (this includes all Exceptions) cannot leak secrets or the content of data. When choosing what to interpolate, it is important to make sure that what is being added is not going to leak secrets or the contents of data. For example, a query with a malformed filter should provide an indication that the filter was malformed and which filter it is, but it cannot include the values being filtered for as that risks leaking data.
+2. Interpolated values should always be encased in a `[]` and come after a noun that describes what is being interpolated. This is to ensure that enough context on what is happening exists and to clearly demark that an interpolation has occurred. Additionally, this identifies the start and end of the interpolation, which is important because messages that attempt to mimic natural prose that also include interpolation can sometimes mask glaring problems (like the inclusion of a space).
+ * Bad: `log.info("%s %s cannot handle %s", "null", "is not null", "INTEGER")` -> `"null is not null cannot handle INTEGER"`
+ * Better, but still not wonderful: `log.info("column[%s] filter[%s] cannot handle type[%s]", "null", "is not null", "INTEGER")` -> `"column[null] filter[is not null] cannot handle type[INTEGER]"`
+ * Good: `log.info("Filter[%s] on column[%s] cannot be applied to type[%s]`, "is not null", "null", "INTEGER")` -> `"Filter[is not null] on column[null] cannot be applied to type[INTEGER]"`
Review Comment:
Good catch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] paul-rogers commented on a diff in pull request #13916: Document our conventions for writing messages
Posted by "paul-rogers (via GitHub)" <gi...@apache.org>.
paul-rogers commented on code in PR #13916:
URL: https://github.com/apache/druid/pull/13916#discussion_r1132620453
##########
dev/style-conventions.md:
##########
@@ -0,0 +1,51 @@
+---
+id: style-conventions
+title: "Druid Style Guide"
+sidebar_label: "Druid Style Guide"
+---
+
+<!--
+ ~ Licensed to the Apache Software Foundation (ASF) under one
+ ~ or more contributor license agreements. See the NOTICE file
+ ~ distributed with this work for additional information
+ ~ regarding copyright ownership. The ASF licenses this file
+ ~ to you under the Apache License, Version 2.0 (the
+ ~ "License"); you may not use this file except in compliance
+ ~ with the License. You may obtain a copy of the License at
+ ~
+ ~ http://www.apache.org/licenses/LICENSE-2.0
+ ~
+ ~ Unless required by applicable law or agreed to in writing,
+ ~ software distributed under the License is distributed on an
+ ~ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ ~ KIND, either express or implied. See the License for the
+ ~ specific language governing permissions and limitations
+ ~ under the License.
+ -->
+
+This document attempts to describe the style that Druid code is expected to follow.
+
+A large amount of the style conventions are handled through IDE configuration and automated checkstyle rules.
+
+- For Intellij you can import our code style settings xml: [`druid_intellij_formatting.xml`](
+ https://github.com/apache/druid/raw/master/dev/druid_intellij_formatting.xml).
+- For Eclipse you can import our code style settings xml: [`eclipse_formatting.xml`](
+ https://github.com/apache/druid/raw/master/dev/eclipse_formatting.xml).
+
+While this page might discuss conventions that are also enforced via said mechanisms, the primary intent is to
+discuss style-related convention that cannot be (or are extremely difficult to be) enforced through such automated
+mechanisms.
+
+## Message Formatting (Logs and Exceptions)
+
+The way that log and exception messages get formatted is an important part of a project. Specifically, it is
+important that there is consistency in formatting such that someone can easily identify and interpret messages.
+This consistency applies to both log *and* exception messages.
+
+1. Messages should have something interpolated into them. Generally speaking, if the time is being taken to generate a message, it is usually valuable to interpolate something from the context into that message. There are exceptions to this, but all messages should start with the assumption that something should be interpolated and try to figure out what that is.
+ * Messages INFO level or above (this includes all Exceptions) cannot leak secrets or the content of data. When choosing what to interpolate, it is important to make sure that what is being added is not going to leak secrets or the contents of data. For example, a query with a malformed filter should provide an indication that the filter was malformed and which filter it is, but it cannot include the values being filtered for as that risks leaking data.
+2. Interpolated values should always be encased in a `[]` and come after a noun that describes what is being interpolated. This is to ensure that enough context on what is happening exists and to clearly demark that an interpolation has occurred. Additionally, this identifies the start and end of the interpolation, which is important because messages that attempt to mimic natural prose that also include interpolation can sometimes mask glaring problems (like the inclusion of a space).
+ * Bad: `log.info("%s %s cannot handle %s", "null", "is not null", "INTEGER")` -> `"null is not null cannot handle INTEGER"`
+ * Better, but still not wonderful: `log.info("column[%s] filter[%s] cannot handle type[%s]", "null", "is not null", "INTEGER")` -> `"column[null] filter[is not null] cannot handle type[INTEGER]"`
Review Comment:
I would recommend a space between the word and the brackets. The brackets are the Druid style for quotes. Hence: `filter [%s]` and not `filter[%s]` because `filter "foo"` and not `filter"foo"`. The one exception is when creating a name, such as a thread name.
##########
dev/style-conventions.md:
##########
@@ -0,0 +1,51 @@
+---
+id: style-conventions
+title: "Druid Style Guide"
+sidebar_label: "Druid Style Guide"
+---
+
+<!--
+ ~ Licensed to the Apache Software Foundation (ASF) under one
+ ~ or more contributor license agreements. See the NOTICE file
+ ~ distributed with this work for additional information
+ ~ regarding copyright ownership. The ASF licenses this file
+ ~ to you under the Apache License, Version 2.0 (the
+ ~ "License"); you may not use this file except in compliance
+ ~ with the License. You may obtain a copy of the License at
+ ~
+ ~ http://www.apache.org/licenses/LICENSE-2.0
+ ~
+ ~ Unless required by applicable law or agreed to in writing,
+ ~ software distributed under the License is distributed on an
+ ~ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ ~ KIND, either express or implied. See the License for the
+ ~ specific language governing permissions and limitations
+ ~ under the License.
+ -->
+
+This document attempts to describe the style that Druid code is expected to follow.
+
+A large amount of the style conventions are handled through IDE configuration and automated checkstyle rules.
+
+- For Intellij you can import our code style settings xml: [`druid_intellij_formatting.xml`](
+ https://github.com/apache/druid/raw/master/dev/druid_intellij_formatting.xml).
+- For Eclipse you can import our code style settings xml: [`eclipse_formatting.xml`](
+ https://github.com/apache/druid/raw/master/dev/eclipse_formatting.xml).
+
+While this page might discuss conventions that are also enforced via said mechanisms, the primary intent is to
+discuss style-related convention that cannot be (or are extremely difficult to be) enforced through such automated
+mechanisms.
+
+## Message Formatting (Logs and Exceptions)
+
+The way that log and exception messages get formatted is an important part of a project. Specifically, it is
+important that there is consistency in formatting such that someone can easily identify and interpret messages.
+This consistency applies to both log *and* exception messages.
+
+1. Messages should have something interpolated into them. Generally speaking, if the time is being taken to generate a message, it is usually valuable to interpolate something from the context into that message. There are exceptions to this, but all messages should start with the assumption that something should be interpolated and try to figure out what that is.
+ * Messages INFO level or above (this includes all Exceptions) cannot leak secrets or the content of data. When choosing what to interpolate, it is important to make sure that what is being added is not going to leak secrets or the contents of data. For example, a query with a malformed filter should provide an indication that the filter was malformed and which filter it is, but it cannot include the values being filtered for as that risks leaking data.
+2. Interpolated values should always be encased in a `[]` and come after a noun that describes what is being interpolated. This is to ensure that enough context on what is happening exists and to clearly demark that an interpolation has occurred. Additionally, this identifies the start and end of the interpolation, which is important because messages that attempt to mimic natural prose that also include interpolation can sometimes mask glaring problems (like the inclusion of a space).
+ * Bad: `log.info("%s %s cannot handle %s", "null", "is not null", "INTEGER")` -> `"null is not null cannot handle INTEGER"`
+ * Better, but still not wonderful: `log.info("column[%s] filter[%s] cannot handle type[%s]", "null", "is not null", "INTEGER")` -> `"column[null] filter[is not null] cannot handle type[INTEGER]"`
+ * Good: `log.info("Filter[%s] on column[%s] cannot be applied to type[%s]", "is not null", "null", "INTEGER")` -> `"Filter[is not null] on column[null] cannot be applied to type[INTEGER]"`
+ * But, if I interpolate an array, I get double `[]`. Yes, that is true, that is okay. Said another way, if a log message contains `[[]]` that is indicative of interpolating an array-like structure.
Review Comment:
Our style guide says to use third person: "But, if you interpolate...".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] paul-rogers commented on pull request #13916: Document our conventions for writing messages
Posted by "paul-rogers (via GitHub)" <gi...@apache.org>.
paul-rogers commented on PR #13916:
URL: https://github.com/apache/druid/pull/13916#issuecomment-1468387332
@imply-cheddar, error messages are for end users. Even when they are for sys admins or devs, they go to end users first. As an end user of many products, I find `column[foo]` to be odd and un-English like. There may well be a good technical reason, but since our audience is end users, and they don't know that technical goal, we should use normally accepted English practice. That should include quotes, but if we choose to use brackets in place of quotes, at least use common spacing.
That said, the code works fine without spaces and users can get used to Druid style. So, LGTM.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] imply-cheddar commented on pull request #13916: Document our conventions for writing messages
Posted by "imply-cheddar (via GitHub)" <gi...@apache.org>.
imply-cheddar commented on PR #13916:
URL: https://github.com/apache/druid/pull/13916#issuecomment-1467123969
@paul-rogers @gianm both of you left comments that I have addressed with updates to the text. Just pinging to give you a chance to double check the adjustments and leave more comments if you do not feel like the new explanations make sense.
The thing I'm mostly wondering about is the explanation about `[]` and it not being the equivalent of quotes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org