Giorgi / DuckDB.NET

Bindings and ADO.NET Provider for DuckDB
https://duckdb.net
MIT License
338 stars 61 forks source link

Make duckdb connection string support unicode path #197

Closed Cricle closed 2 weeks ago

Cricle commented 2 weeks ago

Fix #196

codecov-commenter commented 2 weeks ago

Codecov Report

Attention: Patch coverage is 60.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 88.48%. Comparing base (c6437cc) to head (9b924f6). Report is 27 commits behind head on develop.

Files Patch % Lines
...ET.Bindings/NativeMethods/NativeMethods.Startup.cs 60.00% 3 Missing and 1 partial :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #197 +/- ## =========================================== + Coverage 88.38% 88.48% +0.10% =========================================== Files 54 57 +3 Lines 1756 1841 +85 Branches 239 254 +15 =========================================== + Hits 1552 1629 +77 - Misses 146 153 +7 - Partials 58 59 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

coveralls commented 2 weeks ago

Pull Request Test Coverage Report for Build 9515547530

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
DuckDB.NET.Bindings/NativeMethods/NativeMethods.Startup.cs 7 10 70.0%
<!-- Total: 7 10 70.0% -->
Totals Coverage Status
Change from base Build 9511955148: -0.1%
Covered Lines: 1688
Relevant Lines: 1841

💛 - Coveralls
Cricle commented 2 weeks ago

I think the logic for converting string to handle belongs to the Data project. The Bindings project should contain only method definitions

The NativeMethods.Startup.DuckDBOpen will be a breakchang

Giorgi commented 2 weeks ago

You don't need to delete existing methods, just add a new overload.

Cricle commented 2 weeks ago

If I make it overload method, can accept it?

NativeMethods.Startup.DuckDBOpen(null,out _);//<--- compile error, because the compiler unable to decide what method to execute.
Giorgi commented 2 weeks ago

You can cast null to the desired type to help the compiler.

Cricle commented 2 weeks ago

What about that?

[DllImport(DuckDbLibrary, CallingConvention = CallingConvention.Cdecl, EntryPoint = "duckdb_open")]
public static extern DuckDBState DuckDBOpen(SafeUnmanagedMemoryHandle path, out DuckDBDatabase database);

[DllImport(DuckDbLibrary, CallingConvention = CallingConvention.Cdecl, EntryPoint = "duckdb_open")]
public static extern DuckDBState DuckDBOpen(string? path, out DuckDBDatabase database);

//Usage
NativeMethods.Startup.DuckDBOpen((string?)null,out _);
Giorgi commented 2 weeks ago

That should work.

Cricle commented 2 weeks ago

Completed

Giorgi commented 2 weeks ago

You should not put logic in NativeMethods.Startup, just new overloads and call the correct one from the Data project.

Cricle commented 2 weeks ago

I may not quite understand what you mean, can you explain it in detail?

I know

Giorgi commented 2 weeks ago

Yeah, that's what I meant.