-
Notifications
You must be signed in to change notification settings - Fork 35
Add a section about severity level to the guidelines #90
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
README.adoc
Outdated
| * `WARNING`: The query may have issues. Please review it. We might be able to give you a suggestion on how to improve it. | ||
| * `INFORMATION`: The query may have issues, even though it is correct. The provided information can still be helpful. |
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 would like to also add that we are generally more certain the query have issues for WARNING compared to INFORMATION. Maybe @LinneaAndersson has some feedback too
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.
How about:
WARNING: It is about queries/commands that may have structural, syntactic, semantic, or spelling issues that prevent them from being executed in an optimal way. We need to give example(s) and suggestion(s) for improvement.INFORMATION: It is about queries/commands that are correct but may have some performance issues or that have no effect because they try to do something already done. Depending on the case, we may give example(s) and suggestion(s) for improvement.
Neo.ClientNotification.Database.HomeDatabaseNotFound is the only status code that deviates from the above definition for INFORMATION.
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 think that is better 👍
README.adoc
Outdated
|
|
||
| The severity can be one of the following: | ||
|
|
||
| * `WARNING`: It is about queries/commands that may have structural, syntactic, semantic, or spelling issues that prevent them from being executed in an optimal way. |
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 would more say that:
WARNING: It is about queries/commands that may have structural, syntactic, semantic, or spelling issues where the query most probably is wrong and/or should be updated. This includes: deprecations, experimental features, trying to match on entities with non-existing labels etc. It should be clear what the problem is and it is also good to tell the possible cause, and the consequences, as well as give an example(s) and suggestion(s) for improvement.
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 think the most important part is that "we are (almost) certain it is a problem" and we know how to fix your query. So for:
- Deprecations -> you need to update the query with new functionallity
- Experimental features -> These can not be trusted and should be updated if used in production.
- Non-existing label: This query will never give any result, so it's either useless to run OR misspelled label (unless you add a node with the label, but then the notification would disappear).
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 like that, and I will add it as it is.
|
@LinneaAndersson and @Lojjs, are you happy as it is now? Shall I merge it? |
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 think it looks good, maybe a bit too long? But I leave that up to you to decide :)
README.adoc
Outdated
| * `WARNING`: It is about queries/commands that may have structural, syntactic, semantic, or spelling issues where the query most probably is wrong and/or should be updated. | ||
| This includes deprecations, experimental features, trying to match on entities with non-existing labels, etc. | ||
| It should be clear what the problem is and it is also good to tell the possible cause, and the consequences, as well as give an example(s) and suggestion(s) for improvement. | ||
| I think the most important part is that "we are (almost) certain it is a problem" and we know how to fix your query. So for: |
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 think the most important part is that "we are (almost) certain it is a problem" and we know how to fix your query. So for: | |
| The most important part is that "we are (almost) certain it is a problem" and we know how to fix your query. For example: |
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.
Someone was distracted 😄 and forgot to amend it afterward.
I think it's OK to be long. The more specific the guidelines, the easier it is to follow them. |
No description provided.