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

Remove params parameter from exec function #173

Merged
merged 2 commits into from
Jul 11, 2023
Merged

Remove params parameter from exec function #173

merged 2 commits into from
Jul 11, 2023

Conversation

ar1k
Copy link
Contributor

@ar1k ar1k commented Jul 11, 2023

As exec is not a part of Statement (https://github.com/TryGhost/node-sqlite3/blob/master/src/statement.cc#L17), but a part of Database itself (https://github.com/TryGhost/node-sqlite3/blob/master/src/database.cc#L21), it does not support params binding. So, we should not expose params to the exec public interface here. Also, this corresponds to node-sqlite3 API docs (https://github.com/TryGhost/node-sqlite3/wiki/API#execsql--callback).

@theogravity
Copy link
Collaborator

Thanks, looks good. I'm going to bump a major version as this is a breaking change even if it's invalid.

@theogravity theogravity merged commit be393bb into kriasoft:master Jul 11, 2023
@Cellule
Copy link

Cellule commented Oct 13, 2023

Could this be documented in the CHANGELOG.md file ?
It's very confusing to figure out what's the breaking change

@theogravity
Copy link
Collaborator

theogravity commented Oct 14, 2023

It is documented, but should have been called out as a breaking change. I'll update this.

This project follows semver rules. Assume any major version bump may have a breaking change.

@Cellule
Copy link

Cellule commented Oct 14, 2023

Thank you for the updated changelog, it will make it easier for people to update with confidence 😊

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.

3 participants