Skip to content

Improve documentation of notification config #30

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

Merged
merged 5 commits into from
May 22, 2023

Conversation

robsdedude
Copy link
Member

@robsdedude robsdedude commented May 17, 2023

  • Brings format more in line with other messages and their fields
  • Fixes seeming lack of Bolt 5.0 changes (temporal type representation moved to UTC, etc)
    caused by structure and message changes being listed separately.

@neo-technology-commit-status-publisher

Looks like you've updated the documentation!

Check out your changes at https://neo4j-docs-bolt-30.surge.sh

Message and structure summaries are separated, but should at least link to each
other as they are closely related.
@thelonelyvulpes
Copy link

One aspect that has been missed from the notifications config description is that by disabling a categories or severities means the server doesn't carry out analysis for those.
It may be out of scope of this document but I just thought I'd point it out.

Copy link
Contributor

@stefano-ottolenghi stefano-ottolenghi left a comment

Choose a reason for hiding this comment

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

👙

@robsdedude
Copy link
Member Author

One aspect that has been missed from the notifications config description is that by disabling a categories or severities means the server doesn't carry out analysis for those. It may be out of scope of this document but I just thought I'd point it out.

@thelonelyvulpes That's an interesting point I didn't consider. But I'd argue that these docs are more about the syntax of the protocol and only the semantics inherent to the protocol but not the those semantics defined by the DBMS. Just like we don't describe what Cypher is or how it works here.

On that note tho, this info should be documented somewhere! I'm just not sure where the best place would be? The driver docs? The cypher docs?

@ConorNeo
Copy link
Contributor

One aspect that has been missed from the notifications config description is that by disabling a categories or severities means the server doesn't carry out analysis for those. It may be out of scope of this document but I just thought I'd point it out.

@thelonelyvulpes That's an interesting point I didn't consider. But I'd argue that these docs are more about the syntax of the protocol and only the semantics inherent to the protocol but not the those semantics defined by the DBMS. Just like we don't describe what Cypher is or how it works here.

On that note tho, this info should be documented somewhere! I'm just not sure where the best place would be? The driver docs? The cypher docs?

I agree i think this doc is not the place but should be somewhere!

@stefano-ottolenghi
Copy link
Contributor

I'm just not sure where the best place would be? The driver docs?

I think driver docs is the best place -- or at least, it should be there, and optionally somewhere else as well. I am planning to add a section about notifications in the new manuals, so this will also go in.
I don't think it should be in Cypher since it's not something you can tweak through Cypher.

At the same time, I think there's no harm in dropping a sentence here in Bolt, somewhere. It is informative, and as long as it doesn't digress too much (but again, once sentence, as Grant wrote), it doesn't distract from the rest of docs.

@robsdedude
Copy link
Member Author

robsdedude commented May 22, 2023

@stefano-ottolenghi added a sentence (in 3 places though). What do you think?

@stefano-ottolenghi
Copy link
Contributor

@robsdedude comma between those and which :)

@stefano-ottolenghi stefano-ottolenghi merged commit 9e57fed into main May 22, 2023
@stefano-ottolenghi stefano-ottolenghi deleted the improve-notification-docs branch May 22, 2023 11:57
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.

5 participants