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

feat: support set_id #7

Closed
wants to merge 4 commits into from
Closed

Conversation

huang12zheng
Copy link

fix: test dependencies error with replace _ to -

fix: test dependencies error with replace _ to -
@Aelto
Copy link
Owner

Aelto commented Nov 2, 2022

Thank you for the PR, what is the reason for the changes to the cargo.toml files as well as the renaming of the test file? Using cargo test seems to work on my end without changing anything.

I'd also like to explain what is a label as it seems you have it confused. The label keyword comes from neo4j and its cypher language, you can read an article about it here if you wish. Basically in neo4j the labels are names you add to nodes better identify them. Obviously SurrealQL and cypher are two different things but as SQL is still in its creation phase i decided to use the more mature naming conventions of Cypher. Also note that Cypher has the position of the label & variable inversed, so Account:john in SQL is john:Account in cypher.

With that in mind a function "john".with_label("account") should return account:john and not the opposite, which seems to be the case with your current implementation.

@huang12zheng huang12zheng changed the title feat: add fn with_label #6 [WIP]feat: add fn with_label #6 Nov 2, 2022
@huang12zheng
Copy link
Author

what is the reason for the changes to the cargo.toml files as well as the renaming of the test file

  • surreal-simple-querybuilder\tests\Cargo.toml
  1. Just due to vscode get a error. I replace _ to -
  2. if I just open workspace at tests. it get some error about loss config args about "workspace"

@huang12zheng
Copy link
Author

huang12zheng commented Nov 2, 2022

I'd also like to explain what is a label as it seems you have it confused. The label keyword comes from neo4j and its cypher language, you can read an article about it here if you wish. Basically in neo4j the labels are names you add to nodes better identify them. Obviously SurrealQL and cypher are two different things but as SQL is still in its creation phase i decided to use the more mature naming conventions of Cypher. Also note that Cypher has the position of the label & variable inversed, so Account:john in SQL is john:Account in cypher.

Thank for explain. But I just still think it is need a api(as orm type) to make Model get data with its id. By the way, I use specific instead of label in my earlier draft code.

But this pr generated a String, it is not a good way for me. Because I was actually thinking in an orm way

@Aelto
Copy link
Owner

Aelto commented Nov 2, 2022

Well that doesn't change the fact your notion of "label" is the inverse of what a label actually is. I cannot accept the PR unless the new functions have names that match their behaviour. In Account:John, Account is the label and John is the id. The tests you edited are incorrect now.

The old as_named_label is correct in how it does and name things, if you do Account.as_named_label("John") you expect to get Account as the label and john as the id.

If you wish to have a function that adds a label to a string then i'd suggest a name like "John".with_label("Account") that would give Account:John and not the opposite.

EDIT: Since you are also adding new functions, i'd suggest to write to unit tests rather than remove the tests of other functions to replace them with yours.

Your Name added 2 commits November 2, 2022 23:25
chor: 2. add a test for set_id
refactor: 3. rename with_label to set_id
@huang12zheng huang12zheng changed the title [WIP]feat: add fn with_label #6 feat: add fn with_label #6 Nov 2, 2022
@huang12zheng huang12zheng changed the title feat: add fn with_label #6 feat: support set_id #6 Nov 2, 2022
@huang12zheng huang12zheng changed the title feat: support set_id #6 feat: support set_id Nov 2, 2022
@huang12zheng
Copy link
Author

I refactor it.
I hope to get your feedback

@Aelto
Copy link
Owner

Aelto commented Nov 2, 2022

Can you please describe what is your end goal? You have completely rewritten the PR three time over the course of 24h.

let v = account
    .set_node()
    .set_id("fda")
    .managed_projects()
    .set_id("fda")
    .origin()
    .to_string();

  assert_eq!("Account:fda->manage->Project:fda", v);

this snippet above is from the unit tests you wrote.

Do you wish to be able to build queries like: Label:id->edge->Label:id ? Where nodes each side of the relation have an id? I agree that a with_id function on the ToNodeBuilder trait could be useful. And considering the current implementation of the with function, you could combine them to achieve the following:

let query = account
  .with_id("fda")
  .with(account.managed_projects.with_id("fda"));

assert_eq(query, "Account:fda->manage->Project:fda");

This is possible because the default implementation for the ToNodeBuilder::with function verifies if adding the -> is necessary or not. That means you can pass relations/edges to the with function and it will work as expected.


I am asking this question because i noticed you added two new functions to the generated code by the model macro, while the problem above could be solved by adding a single with_id function to the right trait.

@huang12zheng
Copy link
Author

Close it because I work on the #8.
Thank you again

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.

None yet

2 participants