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

Minor quality of life suggestions #55

Merged
merged 6 commits into from
Nov 30, 2023
Merged

Conversation

simon-curtis
Copy link
Contributor

I have:

  • Added a RawValue property to the Ok result, in case someone wanted to cache the result or do custom deserialisation.
  • Added an options builder delegate extension method .AddSurrealDb(o => o.WithDatabase(...)), it's the type I personally reach for the most when configuring services.

Copy link
Contributor

@Odonno Odonno left a comment

Choose a reason for hiding this comment

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

Looks nice. Some minor changes requested.

SurrealDb.Net/Models/Response/SurrealDbResult.Ok.cs Outdated Show resolved Hide resolved
SurrealDb.Net/Models/Response/SurrealDbResult.Ok.cs Outdated Show resolved Hide resolved
SurrealDb.Net/Models/Response/SurrealDbResult.Ok.cs Outdated Show resolved Hide resolved
@simon-curtis
Copy link
Contributor Author

Regards the RawValue and Deserialising... it might make sense for us to just expose the JsonElement with a private setter?

@simon-curtis
Copy link
Contributor Author

I've made the changes requested. Also I have added a check in the SurrealDbOkResult.DeserializeEnumerable<T>() to ensure that the result of the query was an array. I was going to just yield the object and then return early but if someone is calling DeserializeEnumerable (or GetValues) then I would assume the behaivour they were after was an array. I.e. if they have queried SELECT * FROM ONLY person I would not expect them to call an array method.

Copy link
Contributor

@Odonno Odonno left a comment

Choose a reason for hiding this comment

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

image

@kearfy
Copy link
Member

kearfy commented Nov 30, 2023

Hey @simon-curtis, formatting seems to fail. Could you check that out? Thanks!

@Odonno
Copy link
Contributor

Odonno commented Nov 30, 2023

Indeed, this project is using https://csharpier.com/

You can run it locally in the cli by installing the dotnet tool on your machine. Or automatically in your favorite IDE as an extension. There is one for Rider.

I will update the readme for newcomers.

@simon-curtis
Copy link
Contributor Author

I've formatted but it hasn't made any changes?

@Odonno
Copy link
Contributor

Odonno commented Nov 30, 2023

You have an additional whitespace at the end of line:

public JsonElement Value { get; }

Can you check it isn't there anymore?

@simon-curtis
Copy link
Contributor Author

All done! Codespaces had checkout the wrong branch for some reason. Yes the space after the the property is gone now

@codecov-commenter
Copy link

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (3402154) 70.58% compared to head (a89b9bc) 70.37%.
Report is 1 commits behind head on main.

Files Patch % Lines
...urrealDb.Net/Models/Response/SurrealDbResult.Ok.cs 45.45% 5 Missing and 1 partial ⚠️
...DependencyInjection/ServiceCollectionExtensions.cs 0.00% 5 Missing ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #55      +/-   ##
==========================================
- Coverage   70.58%   70.37%   -0.21%     
==========================================
  Files         109      109              
  Lines        3617     3629      +12     
  Branches      368      369       +1     
==========================================
+ Hits         2553     2554       +1     
- Misses        883      893      +10     
- Partials      181      182       +1     

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

@kearfy kearfy merged commit 352dab3 into surrealdb:main Nov 30, 2023
1 check passed
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.

None yet

4 participants