Skip to content

Update manuals according to Drivers 5.0 changes #335

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 9 commits into from
Oct 21, 2022

Conversation

stefano-ottolenghi
Copy link
Contributor

@stefano-ottolenghi stefano-ottolenghi commented Oct 20, 2022

  • Added a note about execute_read/execute_write replacing read_transaction/write_transaction, or their language-specific counterparts.

  • Made the TrustedCertificates config section language-dependent, and specify the possible values for each language.

  • Removed references to 3.x, except minor informative reminders.

  • Added note about the exception property CanBeRetried.

  • Mention Async Python driver.

  • Waiting for Drivers team to update Java, Javascript and .NET examples about execute_read/write.

Copy link
Contributor

@davidoliverSP2 davidoliverSP2 left a comment

Choose a reason for hiding this comment

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

Thanks @stefano-ottolenghi

Just a couple of small things to look at.

@@ -1486,19 +1459,13 @@ include::{python-examples}/test_config_max_retry_time_example.py[tags=config-max

# tag::configuration-TrustStrategy[]

`TrustStrategy`::
`TrustedCertificates`::
Copy link
Contributor

Choose a reason for hiding this comment

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

The example in this renamed section still uses TrustStrategy

I guess if this is an error, it's not a documentation fix, and as such shouldn't delay this PR.

Screenshot 2022-10-20 at 14 26 18

Copy link
Contributor Author

@stefano-ottolenghi stefano-ottolenghi Oct 20, 2022

Choose a reason for hiding this comment

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

tl;dr - It is a wrong label anyway, I can restore it to TrustStrategy if we want to avoid a pointless diff.

Right, so this is an annoying bit that basically has to do with the fact that different drivers have different option names, so that TrustedCertificates (or TrustStrategy, for that matter) is inaccurate for them all. This problem applies to all configuration options in that page.

What happened is that I started reviewing from the Python manual, which calls is trusted_certificates, so I renamed the section to TrustedCertificates - still wrong, but at least with the right words. Then I discovered that each language has it different (for ex, in the .NET screenshot here, it is WithTrustManager), so the correction is pointless. IIRC the Java driver is the only one having TrustStrategy, which would explain at least historically why that section had that name. Then Drivers diverged and we kept the same naming for all of them.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this is a good case for bringing this content into the individual language folders, and out of the common-content folder?

I think that could be done in another PR though, and shouldn't hold us up on this PR.

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, I would defer this to the re-design of the manuals :)

@stefano-ottolenghi stefano-ottolenghi merged commit 4310757 into neo4j:dev Oct 21, 2022
@stefano-ottolenghi
Copy link
Contributor Author

Related to
neo4j/neo4j-java-driver#1322
neo4j/neo4j-javascript-driver#1009
neo4j/neo4j-dotnet-driver#652
(Python and Go were already up to date)

@stefano-ottolenghi stefano-ottolenghi deleted the update-manuals-5.0 branch October 21, 2022 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants