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

Geal/merge main in v5 #238

Merged
merged 4 commits into from
Nov 19, 2024
Merged

Geal/merge main in v5 #238

merged 4 commits into from
Nov 19, 2024

Conversation

Geal
Copy link
Contributor

@Geal Geal commented Nov 3, 2024

No description provided.

ptondereau and others added 2 commits November 3, 2024 18:38
* refactor(capi): Move CAPI generation to its own crate

* reduce Biscuit visibility

* remove capi.rs

* add `print_block_source` c binding

* add test for `biscuit_print_block_source` + use `inttypes.h` macros for handling cross-platform `uint64_t` type

* remove `block()` usage and C-API usage
* do not build on x86 OSX

* update readme

* Apply suggestions from code review
@Geal Geal changed the base branch from main to v5 November 3, 2024 20:35
Copy link

codspeed-hq bot commented Nov 3, 2024

CodSpeed Performance Report

Merging #238 will not alter performance

Comparing geal/merge-main-in-v5 (ed7cb5b) with v5 (e59f41d)

Summary

✅ 12 untouched benchmarks

Copy link

codecov bot commented Nov 19, 2024

Codecov Report

Attention: Patch coverage is 0% with 35 lines in your changes missing coverage. Please review.

Project coverage is 64.79%. Comparing base (3930767) to head (ed7cb5b).
Report is 8 commits behind head on v5.

Files with missing lines Patch % Lines
biscuit-capi/src/lib.rs 0.00% 35 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##               v5     #238      +/-   ##
==========================================
- Coverage   69.03%   64.79%   -4.24%     
==========================================
  Files          25       25              
  Lines        5926     6502     +576     
==========================================
+ Hits         4091     4213     +122     
- Misses       1835     2289     +454     

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


🚨 Try these New Features:

Copy link
Collaborator

@divarvel divarvel left a comment

Choose a reason for hiding this comment

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

Looks good. My comments are not blocking, since all this has been merged to main anyway

@@ -2,69 +2,55 @@ name: Rust

on:
push:
branches: [ main ]
branches: [main]
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not v5 as well? we run CI on all PRs targeting v5, but not on the v5 PR itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point. I think I need to fix this in a PR for main though

@@ -0,0 +1,30 @@
[package]
name = "biscuit-capi"
version = "5.0.0" # Should keep the same version as biscuit-auth
Copy link
Collaborator

Choose a reason for hiding this comment

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

why all the spaces before the comments?


[features]
default = []
capi = ["inline-c"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

should the capi crate have an optional capi feature?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that can be removed. inline-c is only used in the capi tests. I'll remove it in main first then port to v5

@@ -0,0 +1,36 @@
## Biscuit C API

This crate provides a C API for the Biscuit token library. It is a wrapper around the Rust API, and is intended to be used by other languages that can interface with C.
Copy link
Collaborator

Choose a reason for hiding this comment

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

by C or other languages that can interface with C. If a language provides rust bindings through C (for instance Python with Maturin), it may be better to use that rather than this library.

@Geal
Copy link
Contributor Author

Geal commented Nov 19, 2024

I'll make a follow up PR to main to address those comments

@Geal Geal merged commit 796ba5d into v5 Nov 19, 2024
5 of 7 checks 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.

3 participants