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

Sqlite Node - docs update #869

Merged
merged 8 commits into from Jan 5, 2022
Merged

Sqlite Node - docs update #869

merged 8 commits into from Jan 5, 2022

Conversation

unborn-andy
Copy link
Contributor

Types of changes

Updated sqlite node README.md to match the more detailed node html file (the usage info was already there)
This can give better information to users that dont have the node installed but want to see how it works from the readme.md file.

Also added an example of how to use parameters Via msg.topic for both node html and README.md

1. Updated node readme to match node's html help.
2. Added Example clarifying the use of parameters in a msg.topic query.
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 4, 2022

CLA Signed

The committers are authorized under a signed CLA.

@dceejay
Copy link
Member

dceejay commented Jan 5, 2022

Hi @unborn-andy - Any chance you can click/sign the CLA so we can accept and merge this PR ? Thanks

@unborn-andy
Copy link
Contributor Author

Hello @dceejay. I did the CLA.

By the way i just saw that @hardillb pushed a fix for the sqlite node parameters with the reseting of the bind. good job
(I didnt catch them on time to update the docs aswell)

Hope this PR is ok .. its my first one ;)
Thanks

@dceejay
Copy link
Member

dceejay commented Jan 5, 2022

Hi - yes look fine to me... - did you want to update it before I merge it in light of the other PR ?

@unborn-andy
Copy link
Contributor Author

did you want to update it before I merge it in light of the other PR ?

what do you mean ?
As you saw the changes i made dont affect the fix Ben did.
If we merge my PR does this mean we have to increment the node-red-node-sqlite version once more to 1.0.3 ?
(By the way i didnt change any version in package.json or anything in case its needed)

@dceejay
Copy link
Member

dceejay commented Jan 5, 2022

Hi - yes I will have to bump the version but that is OK... it's better that you don't bump the version and leave that for me - for exactly the reason you spotted - ie if I wanted to add multiple changes into the one release...

I was just wondering if you wanted to add to the info.
(As it was just a fix then probably not - but I thought I saw something re % and $ that someone mentioned and you wanted to clarify).

@unborn-andy
Copy link
Contributor Author

I thought I saw something re % and $ that someone mentioned and you wanted to clarify

yes .. i asked in the Forum thread about this ..
i was reading a bit more about the sqlite parameters and there are a few examples on the library's Wiki page. Ex.

      // Directly in the function arguments.
      db.run("UPDATE tbl SET name = ? WHERE id = ?", "bar", 2);

      // As an array.
      db.run("UPDATE tbl SET name = ? WHERE id = ?", [ "bar", 2 ]);

      // As an object with named parameters.
      db.run("UPDATE tbl SET name = $name WHERE id = $id", {
          $id: 2,
          $name: "bar"
      });

So based on the above examples .. we can pass params as named params with the $ syntax but ? is also valid.
In older versions of the node the ? systax was working (as reported by the OP) until you introduced that extra check in the code
if (msg.payload.length === (msg.topic.split('$').length - 1) ) { bind = msg.payload; }

Maybe we can accept both ways of passing parms. If its worth it ;)

if ((msg.payload.length === (msg.topic.split('$').length - 1)) || (msg.payload.length === (msg.topic.split('?').length - 1)) ) {
    bind = msg.payload;
}

@dceejay
Copy link
Member

dceejay commented Jan 5, 2022

Ah no they are different - first passes an array of values the second passes and object with named properties.
Let's merge this as-is then and can debate this other point as a separate issue/thread.

@dceejay dceejay merged commit 7ff905d into node-red:master Jan 5, 2022
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