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 force try in the FailableIterator.next() and Connection.prepare() methods #1273

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

syoung-smallwisdom
Copy link

The force try is crashing and ideally, the prepare() query should throw the error rather than crashing the app. Thanks!

Thanks for taking the time to submit a pull request.

Before submitting, please do the following:

  • Run make lint to check if there are any format errors (install swiftlint first)
  • Run swift test to see if the tests pass.
  • Write new tests for new functionality.
  • Update documentation comments where applicable.

@jokerYellow
Copy link

anyone please merge this request?

@dyikai
Copy link
Contributor

dyikai commented Jan 9, 2025

Why don't you just use prepareRowIterator instead of prepare?

@syoung-smallwisdom
Copy link
Author

Why don't you just use prepareRowIterator instead of prepare?

I don't understand the question. The prepare() method is a publicly exposed method that is included in the README as an example usage of the library.

@dyikai
Copy link
Contributor

dyikai commented Jan 9, 2025

I notice this line was reverted from try? in the past, this may be the reason #1075 and the previous fix pr is #1030

@syoung-smallwisdom
Copy link
Author

syoung-smallwisdom commented Jan 9, 2025

I notice this line was reverted from try? in the past, this may be the reason #1075 and the previous fix pr is #1030

If you read through the comments for why:
#1030 - Don't crash and documentation should be updated.
#1075 - Revert the change b/c it causes an error in prepare().

Note that the documentation was never updated to tell users that "If you use this method that we included in the library and use in the example README, then it can cause your production to app crash". Seems to me that there are two solutions to this:

  1. Mark the method as unavailable and don't implement the Iterator protocol.
    OR
  2. Fix it by making the next() method adhere to the protocol and fixing prepare() (which can throw errors) to use failableNext() instead.

I chose Option 2. It's up to those who still use this fork of the repo to decide if they want to accept the fix - I would certainly suggest that if the community would like to keep people from forking off and maintaining their own forks, then "just use a different method that is not in the documented README" is not a solution.

FWIW, anyone who wants this fix is welcome to use my client's fork: https://github.com/BiAffectBridge/SQLite.swift

@xiushaomin
Copy link

any update?

@arthursalgado
Copy link

Any update?

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.

5 participants