Skip to content

Conversation

@imrn99
Copy link
Collaborator

@imrn99 imrn99 commented Nov 13, 2024

Sorry for the size of the last 2 PRs!

New semantic over attributes:

Done:

  • add a new enum, CMapError
  • add type alias CMapResult<T> = Result<T, CMapError>
  • update embed methods to use the new semantic
  • update the AttributeStorage<A> & UnknownAttributeStorage to use the new semantic
    • also update the trait implementation for AttrSparseVec
  • update AttrStorageManager to use the new semantic
  • update the documentation with explanation over the new semantic

Follow-up:

  • add tests over edge case allowed by the new semantics (e.g. a potential transaction cancel over some concurrent changes)
  • replace some options with with CMapResult<A> where possible / coherent
    • this corresponds mostly to places with CMapResult<Option<A>>
  • update AttributeUpdate fallback methods to return CMapResult<Self>
    • this creates even more control, with some attributes being essential and others fail-proof

@codecov-commenter
Copy link

codecov-commenter commented Nov 19, 2024

Codecov Report

Attention: Patch coverage is 73.66185% with 310 lines in your changes missing coverage. Please review.

Project coverage is 79.21%. Comparing base (81f9a53) to head (d5d4831).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
honeycomb-core/src/attributes/manager.rs 42.42% 171 Missing ⚠️
honeycomb-core/src/attributes/collections.rs 50.60% 41 Missing ⚠️
honeycomb-core/src/cmap/dim2/embed.rs 30.90% 38 Missing ⚠️
honeycomb-core/src/cmap/dim2/sews/two.rs 47.27% 29 Missing ⚠️
honeycomb-core/src/cmap/dim2/sews/one.rs 46.66% 8 Missing ⚠️
honeycomb-kernels/src/grisubal/routines/clip.rs 0.00% 8 Missing ⚠️
honeycomb-core/src/attributes/traits.rs 50.00% 6 Missing ⚠️
honeycomb-core/src/cmap/error.rs 0.00% 3 Missing ⚠️
honeycomb-core/src/cmap/dim2/links/one.rs 0.00% 2 Missing ⚠️
honeycomb-core/src/cmap/dim2/links/two.rs 0.00% 2 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #226      +/-   ##
==========================================
- Coverage   80.67%   79.21%   -1.46%     
==========================================
  Files          51       52       +1     
  Lines        6850     6953     +103     
==========================================
- Hits         5526     5508      -18     
- Misses       1324     1445     +121     

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


🚨 Try these New Features:

@imrn99 imrn99 changed the title refactor!(core): rework all attribute-related items refactor!(core): rework attribute semantics Nov 19, 2024
@imrn99 imrn99 marked this pull request as ready for review November 19, 2024 14:15
@imrn99 imrn99 mentioned this pull request Nov 19, 2024
5 tasks
Copy link
Member

@cedricchevalier19 cedricchevalier19 left a comment

Choose a reason for hiding this comment

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

Looks ok, I will mainly trust the Rust toolchain on this PR.

Comment on lines +37 to 39
/// validate the transaction passed as argument. The result should not be processed manually,
/// only used via the `?` operator.
///
Copy link
Member

Choose a reason for hiding this comment

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

I do not really follow? Do you mean that even if the result value is not interesting, the user has to check if ok?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe the stm crate uses these errors to detect invalid transaction (early?), so yes, the ? operator should be called on every results inside a transaction, including Result<(), StmError>.

@imrn99
Copy link
Collaborator Author

imrn99 commented Nov 23, 2024

Looks ok, I will mainly trust the Rust toolchain on this PR.

We can set something up this week to go over the API and any questions / shortcomings.

@imrn99 imrn99 merged commit a924645 into master Nov 24, 2024
13 checks passed
@imrn99 imrn99 deleted the better-attribute-collections branch November 24, 2024 18:26
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.

4 participants