Skip to content
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

Fix comment filtering on command #1140

Merged
merged 1 commit into from
Jul 16, 2020

Conversation

OskarDamkjaer
Copy link
Contributor

@OskarDamkjaer OskarDamkjaer commented Jul 14, 2020

Replaced the comment regex with a filter that easier to understand and doesn't have this issue:
image

However. This is an edge-case but could happen:
image
It seems to me from https://neo4j.com/docs/2.2.2/cypher-comments.html that cypher supports comments, any reason why we need to "clean the command"? Could it not simply be removed (perhaps some version stuff with cypher?).

From some initial looks this
image
Doesn't work if we let cypher handle the comments because then the second statement is evaluated as cypher instead of a command. So perhaps leaving the cleaning as it is is preferable? The current implementation has that awkward edgecase but at least the syntax highlighting indicates the something might be wrong.

changelog: Fix comment issue in multi statement commands

@OskarDamkjaer OskarDamkjaer requested review from huboneo and pe4cey July 14, 2020 13:10
Copy link
Contributor

@huboneo huboneo left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@pe4cey pe4cey left a comment

Choose a reason for hiding this comment

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

This is something for the future, but I think it's worth considering extending the cypher-codemirror component to strip the comments out (although the comments are used as the name for favourited scripts 🤔 )

@OskarDamkjaer
Copy link
Contributor Author

@pe4cey Code mirror does strip comments but it's seems it just doesn't do it correctly all the time. We're moving away from codemirror so I thought it'd take to long to fix it in that code base instead

@OskarDamkjaer OskarDamkjaer merged commit 4a01db7 into neo4j:master Jul 16, 2020
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.

4 participants