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

Enable REGEXP by connecting Crystal's stdlib Regex #62

Merged
merged 1 commit into from Nov 11, 2020
Merged

Enable REGEXP by connecting Crystal's stdlib Regex #62

merged 1 commit into from Nov 11, 2020

Conversation

ghost
Copy link

@ghost ghost commented Nov 10, 2020

For #61.

Phew! This took me a long time to figure out. I've never worked with Crystal's FFI before. Ideally this should expose the create_function interface fully, but I hope to get feedback on the code before I try to take it farther. I can add tests tomorrow.

Copy link
Member

@bcardiff bcardiff left a comment

Choose a reason for hiding this comment

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

Looking good! We would need some specs to ensure things are working and will not break.

:memory: are available to use or you can add them at spec/db_spec.cr where the it block provides an empty db already.

src/sqlite3/lib_sqlite3.cr Outdated Show resolved Hide resolved
src/sqlite3/connection.cr Outdated Show resolved Hide resolved
@@ -87,3 +89,11 @@ class SQLite3::Connection < DB::Connection
raise Exception.new(self) unless code == 0
end
end

REGEXP_FN = ->(context : LibSQLite3::SQLite3Context, argc : Int32, argv : LibSQLite3::SQLite3Value*) do
Copy link
Member

Choose a reason for hiding this comment

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

Great, but let's leave this constant within SQLite3 namespace.

module SQLite3
  # :nodoc:
  REGEXP_FN = ->(context : ...
end

It's better if placed either at the beginning of this file or at src/sqlite3.cr. Either will work.

Other than that, LGTM!

Copy link
Author

Choose a reason for hiding this comment

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

Thanks! I put it in src/sqlite3.cr.

@bcardiff bcardiff merged commit 55b8399 into crystal-lang:master Nov 11, 2020
@bcardiff
Copy link
Member

Thanks @yujiri8 🙇

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.

1 participant