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

Make duckdb connection string support unicode path #197

Merged
merged 1 commit into from
Jun 14, 2024

Conversation

Cricle
Copy link
Contributor

@Cricle Cricle commented Jun 14, 2024

Fix #196

@Cricle Cricle changed the title Fix #196 Make duckdb connection string support unicode path Jun 14, 2024
@codecov-commenter
Copy link

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 ⚠️

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

Additional details and impacted files
@@             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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@coveralls
Copy link

coveralls commented Jun 14, 2024

Pull Request Test Coverage Report for Build 9515547530

Details

  • 7 of 10 (70.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.1%) to 90.435%

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

💛 - Coveralls

Copy link
Owner

@Giorgi Giorgi left a comment

Choose a reason for hiding this comment

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

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

@Cricle
Copy link
Contributor Author

Cricle commented Jun 14, 2024

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
Copy link
Owner

Giorgi commented Jun 14, 2024

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

@Cricle
Copy link
Contributor Author

Cricle commented Jun 14, 2024

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
Copy link
Owner

Giorgi commented Jun 14, 2024

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

@Cricle
Copy link
Contributor Author

Cricle commented Jun 14, 2024

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
Copy link
Owner

Giorgi commented Jun 14, 2024

That should work.

@Cricle
Copy link
Contributor Author

Cricle commented Jun 14, 2024

Completed

@Giorgi
Copy link
Owner

Giorgi commented Jun 14, 2024

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

@Cricle
Copy link
Contributor Author

Cricle commented Jun 14, 2024

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

I know

@Giorgi
Copy link
Owner

Giorgi commented Jun 14, 2024

Yeah, that's what I meant.

@Giorgi Giorgi merged commit 87187a2 into Giorgi:develop Jun 14, 2024
1 check failed
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.

Can't use unicode for connectionstring
4 participants