Skip to content

Conversation

@Hunterness
Copy link
Contributor

No description provided.

@renetapopova renetapopova self-assigned this Apr 2, 2025
@neo-technology-commit-status-publisher
Copy link
Collaborator

This PR includes documentation updates
View the updated docs at https://neo4j-docs-operations-2216.surge.sh

Updated pages:

@renetapopova renetapopova changed the base branch from dev to cypher-25 April 2, 2025 10:55
@Hunterness Hunterness added team-cypher-operations Cypher operations should review this Cypher 25 labels Apr 2, 2025
@Hunterness Hunterness marked this pull request as ready for review April 2, 2025 13:20
@renetapopova renetapopova self-requested a review April 3, 2025 09:40
Copy link
Collaborator

@renetapopova renetapopova left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Copy link
Contributor

@Lojjs Lojjs left a comment

Choose a reason for hiding this comment

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

The main thing looks fine, just two comments on some wording which might be misleading. I did not test to build it locally, only checked the PR.

== Database alias management command syntax

[options="header", width="100%", cols="1,5a"]
The following tables cover both local and remote database aliases.
Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding is that it is only the very next table which cover both, after that it is separate tables for local and remote since there are some variations. So I think you either want to write just "The following table..." instead of "The following tables..." or clarify it a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's the general tables as in all of these tables together covers both? but that sentence came from @renetapopova so I'll let her decide if we want to update it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is it better to just remove the both?

Suggested change
The following tables cover both local and remote database aliases.
The following tables cover local and remote database aliases.

Copy link
Contributor

Choose a reason for hiding this comment

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

No I do not think removing 'both' helps. I interpret this sentence as each of the following tables (unclear how many) covers both local and remote alias, which is not true. Some tables cover both, some cover local and some cover remote if I understand the layout of the page correctly

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it is just me that interpret the sentence incorrectly, if you think it is correct already I am fine with leaving it. Same for my other comment which is about the same thing but in another place

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
The following tables cover both local and remote database aliases.
The following sections cover the commands related to local and remote database aliases.

is that better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or feel free to come up with your own suggestion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the sentence isn't supposed to talk about just the next one so changing to The following table isn't the way to go

Copy link
Collaborator

@renetapopova renetapopova left a comment

Choose a reason for hiding this comment

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

Added new suggestions.

Copy link
Contributor

@Lojjs Lojjs left a comment

Choose a reason for hiding this comment

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

Looks good now

@renetapopova renetapopova merged commit e4d42e6 into neo4j:cypher-25 Apr 4, 2025
@Hunterness Hunterness deleted the dev-add-optional-set branch April 4, 2025 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Cypher 25 team-cypher-operations Cypher operations should review this

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants