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

Add new param syntax to evaluate against the server #740

Merged
merged 18 commits into from
Apr 23, 2018

Conversation

pe4cey
Copy link
Contributor

@pe4cey pe4cey commented Apr 17, 2018

Summary

Introduces additional :param syntax that evaluates the expression against neo4j server

More info

The statement to the right of => will be evaluated against neo4j server (as cypher) and the resulting value stored against the param name to be used in future cypher statements (e.g. RETURN $name).

NOTE: Setting params using :param foo: "bar"is still supported but will not be executed against the neo4j server

Examples

Browser command - return value (type)

:param foo => "bar" ->"bar" (string)
:param foo => 1 + 1 ->2 (bolt int)
:param foo => date() -> "2018-04-23" (bolt date string)

Fixes #705

@pe4cey pe4cey force-pushed the param-fn branch 2 times, most recently from 2e95d17 to db1788c Compare April 18, 2018 12:35
@pe4cey pe4cey removed the DONT-MERGE label Apr 18, 2018
Copy link
Member

@oskarhane oskarhane left a comment

Choose a reason for hiding this comment

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

Nice!
Just a few comments on things that could be changed.

@@ -137,6 +144,12 @@ describe('commandsDuck', () => {
const cmdString = cmd + ' x: 2'
const id = 1
const action = commands.executeCommand(cmdString, id)
bolt.routedWriteTransaction = jest.fn(() =>
Copy link
Member

Choose a reason for hiding this comment

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

This is needed here, because this should not call that function.

@@ -175,6 +189,7 @@ describe('commandsDuck', () => {
{ type: commands.KNOWN_COMMAND },
replaceParams({ x: 2, y: 3 }),
frames.add({ ...action, success: true, type: 'params', params: {} }),
{ type: 'meta/FORCE_FETCH' },
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't force fetch on param setting

return reject(
new Error('Could not parse input. Usage: `:param "x": 2`. ' + e)
)
reject(new Error('Could not parse input. Usage: `:param "x": 2`. ' + e))
Copy link
Member

Choose a reason for hiding this comment

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

I think we should promote the new format here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Display both usages or only the new => approach?

Copy link
Member

Choose a reason for hiding this comment

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

Both, I think.

@@ -44,6 +50,8 @@ describe('commandsDuck params helper', () => {
})
test('handles :param "x": 2 and calls the update action creator', () => {
// Given
// jest.spyOn(params, , accessType?)
Copy link
Member

Choose a reason for hiding this comment

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

not needed

@@ -90,7 +91,7 @@ const availableCommands = [
name: 'set-params',
match: cmd => /^params?\s/.test(cmd),
exec: function (action, cmdchar, put, store) {
handleParamsCommand(action, cmdchar, put)
return handleParamsCommand(action, cmdchar, put, bolt)
Copy link
Member

Choose a reason for hiding this comment

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

It looks like handleParamsCommand isn't using the 'bolt argument so it can be skipped here (and also the import up top)

it('will add parameter using `:param` command', () => {
// add cypher evalutated param function
const command = ':param foo => 1 + 1'
cy.get(Editor).type(command, { force: true })
Copy link
Member

Choose a reason for hiding this comment

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

Nice with e2e test!
You could use cy.executeCommand(command) here instead to clean up a bit.

@@ -28,7 +28,9 @@ const ParamsFrame = ({ frame, params }) => {
const contents = (
<PaddedDiv>
<Render if={frame.success !== false}>
<pre>{JSON.stringify(frame.params, null, 2)}</pre>
<pre data-test-id='rawParamData'>
{JSON.stringify(frame.params, null, 2)}
Copy link
Member

Choose a reason for hiding this comment

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

I think we want a nicer output for the new types. Might be in a separate PR, let's chat about it.

@@ -43,6 +43,10 @@ import {
replace as replaceSettings
} from 'shared/modules/settings/settingsDuck'
import { cleanCommand, getInterpreter } from 'services/commandUtils'
import bolt from 'services/bolt/bolt'
Copy link
Member

Choose a reason for hiding this comment

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

Bolt should not be needed in this file since we're not testing the new format, right?

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've just added a test in the latest commit that requires this.

@pe4cey
Copy link
Contributor Author

pe4cey commented Apr 20, 2018

PR updated @oskarhane

@oskarhane
Copy link
Member

In #735 I've added a new function that formats all types in a nice way. It'd be great if that could be used when printing the params as well.

test('does the right thing for :param x => 2', done => {
// Given
const cmd = store.getState().settings.cmdchar + 'param'
const cmdString = cmd + ' x: 2'
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't test what the test title says it should test.

@oskarhane oskarhane merged commit c2408b2 into neo4j:master Apr 23, 2018
myzero1 pushed a commit to myzero1/neo4j-browser that referenced this pull request May 17, 2019
Add new param syntax to evaluate against the server
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